Bruce Momjian writes:
Add GUC temp_tablespaces to provide a default location for temporary
objects.
Jaime Casanova
I hadn't looked at this patch before, but now that I have, it is
rather broken.

In the first place, it makes no provision for RemovePgTempFiles() to
clean up leftover temp files that are in non-default places.

In the second place, it's a serious violation of what little modularity
and layering we have for fd.c to be calling into commands/tablespace.c.
This is not merely cosmetic but has real consequences: one being that
it's now unsafe to call OpenTemporaryFile outside a transaction.

Please revert until the submitter can come up with a better-designed
patch.

regards, tom lane

Search Discussions

  • Jaime Casanova at Mar 17, 2007 at 3:10 pm

    On 3/5/07, Tom Lane wrote:
    In the second place, it's a serious violation of what little modularity
    and layering we have for fd.c to be calling into commands/tablespace.c.
    This is not merely cosmetic but has real consequences: one being that
    it's now unsafe to call OpenTemporaryFile outside a transaction.
    ok, you are right... what do you suggest?
    maybe move the GetTempTablespace function to somewhere in src/backend/utils?

    --
    regards,
    Jaime Casanova

    "Programming today is a race between software engineers striving to
    build bigger and better idiot-proof programs and the universe trying
    to produce bigger and better idiots.
    So far, the universe is winning."
    Richard Cook
  • Tom Lane at Mar 17, 2007 at 3:36 pm

    "Jaime Casanova" <systemguards@gmail.com> writes:
    On 3/5/07, Tom Lane wrote:
    In the second place, it's a serious violation of what little modularity
    and layering we have for fd.c to be calling into commands/tablespace.c.
    This is not merely cosmetic but has real consequences: one being that
    it's now unsafe to call OpenTemporaryFile outside a transaction.
    ok, you are right... what do you suggest?
    maybe move the GetTempTablespace function to somewhere in src/backend/utils?
    You missed the point entirely. Relocating the code to some other file
    wouldn't change the objection: the problem is that fd.c mustn't invoke
    any transactional facilities such as catalog lookups. It's too low
    level for that.

    You could perhaps do it the other way around: some transactional
    code (eg the assign hook for a GUC variable) tells fd.c to save
    some private state controlling future temp file creations.

    BTW, if we are now thinking of temp files as being directed to
    particular tablespaces, is there any reason still to have per-database
    subdirectories for them? It might simplify life if there were just
    one default temp directory, $PGDATA/base/pgsql_tmp/, plus one per
    configured temp tablespace, $PGDATA/pg_tblspc/NNNN/pgsql_tmp/.

    regards, tom lane
  • Jaime Casanova at Mar 18, 2007 at 7:05 pm

    On 3/17/07, Tom Lane wrote:
    "Jaime Casanova" <systemguards@gmail.com> writes:
    On 3/5/07, Tom Lane wrote:
    In the second place, it's a serious violation of what little modularity
    and layering we have for fd.c to be calling into commands/tablespace.c.
    This is not merely cosmetic but has real consequences: one being that
    it's now unsafe to call OpenTemporaryFile outside a transaction.
    ok, you are right... what do you suggest?
    maybe move the GetTempTablespace function to somewhere in src/backend/utils?
    You missed the point entirely. Relocating the code to some other file
    wouldn't change the objection: the problem is that fd.c mustn't invoke
    any transactional facilities such as catalog lookups. It's too low
    level for that.
    oh, i see...
    You could perhaps do it the other way around: some transactional
    code (eg the assign hook for a GUC variable) tells fd.c to save
    some private state controlling future temp file creations.
    the problem with the assign hook function is that it can't read
    catalogs too if we are in a non-interactive command...

    so, we need a list with the oids of the tablespaces, we can initialize
    this list the first time we need a temp file (i haven't seen exactly
    where we can do this, yet), and if we set the GUC via a SET command
    then we can let the assign hook do the job.
    BTW, if we are now thinking of temp files as being directed to
    particular tablespaces, is there any reason still to have per-database
    subdirectories for them? It might simplify life if there were just
    one default temp directory, $PGDATA/base/pgsql_tmp/, plus one per
    configured temp tablespace, $PGDATA/pg_tblspc/NNNN/pgsql_tmp/.
    what the NNNN directory shoud be, i understand ypur idea as just
    $PGDATA/pg_tblspc/pgsql_tmp/...

    --
    regards,
    Jaime Casanova

    "Programming today is a race between software engineers striving to
    build bigger and better idiot-proof programs and the universe trying
    to produce bigger and better idiots.
    So far, the universe is winning."
    Richard Cook
  • Simon Riggs at Apr 2, 2007 at 5:48 pm

    On Sun, 2007-03-18 at 14:05 -0500, Jaime Casanova wrote:
    On 3/17/07, Tom Lane wrote:
    "Jaime Casanova" <systemguards@gmail.com> writes:
    On 3/5/07, Tom Lane wrote:
    In the second place, it's a serious violation of what little modularity
    and layering we have for fd.c to be calling into commands/tablespace.c.
    This is not merely cosmetic but has real consequences: one being that
    it's now unsafe to call OpenTemporaryFile outside a transaction.
    ok, you are right... what do you suggest?
    maybe move the GetTempTablespace function to somewhere in src/backend/utils?
    You missed the point entirely. Relocating the code to some other file
    wouldn't change the objection: the problem is that fd.c mustn't invoke
    any transactional facilities such as catalog lookups. It's too low
    level for that.
    oh, i see...
    You could perhaps do it the other way around: some transactional
    code (eg the assign hook for a GUC variable) tells fd.c to save
    some private state controlling future temp file creations.
    the problem with the assign hook function is that it can't read
    catalogs too if we are in a non-interactive command...

    so, we need a list with the oids of the tablespaces, we can initialize
    this list the first time we need a temp file (i haven't seen exactly
    where we can do this, yet), and if we set the GUC via a SET command
    then we can let the assign hook do the job.
    BTW, if we are now thinking of temp files as being directed to
    particular tablespaces, is there any reason still to have per-database
    subdirectories for them? It might simplify life if there were just
    one default temp directory, $PGDATA/base/pgsql_tmp/, plus one per
    configured temp tablespace, $PGDATA/pg_tblspc/NNNN/pgsql_tmp/.
    what the NNNN directory shoud be, i understand ypur idea as just
    $PGDATA/pg_tblspc/pgsql_tmp/...
    Am I right in thinking we didn't get an updated patch in yet?

    Any help needed here?

    This seems a very important patch for manageability and it would be a
    shame to miss out on it for 8.3 since its been a TODO item for so long.

    --
    Simon Riggs
    EnterpriseDB http://www.enterprisedb.com
  • Bruce Momjian at Apr 2, 2007 at 5:55 pm
    Right, no updated patch submitted.

    ---------------------------------------------------------------------------

    Simon Riggs wrote:
    On Sun, 2007-03-18 at 14:05 -0500, Jaime Casanova wrote:
    On 3/17/07, Tom Lane wrote:
    "Jaime Casanova" <systemguards@gmail.com> writes:
    On 3/5/07, Tom Lane wrote:
    In the second place, it's a serious violation of what little modularity
    and layering we have for fd.c to be calling into commands/tablespace.c.
    This is not merely cosmetic but has real consequences: one being that
    it's now unsafe to call OpenTemporaryFile outside a transaction.
    ok, you are right... what do you suggest?
    maybe move the GetTempTablespace function to somewhere in src/backend/utils?
    You missed the point entirely. Relocating the code to some other file
    wouldn't change the objection: the problem is that fd.c mustn't invoke
    any transactional facilities such as catalog lookups. It's too low
    level for that.
    oh, i see...
    You could perhaps do it the other way around: some transactional
    code (eg the assign hook for a GUC variable) tells fd.c to save
    some private state controlling future temp file creations.
    the problem with the assign hook function is that it can't read
    catalogs too if we are in a non-interactive command...

    so, we need a list with the oids of the tablespaces, we can initialize
    this list the first time we need a temp file (i haven't seen exactly
    where we can do this, yet), and if we set the GUC via a SET command
    then we can let the assign hook do the job.
    BTW, if we are now thinking of temp files as being directed to
    particular tablespaces, is there any reason still to have per-database
    subdirectories for them? It might simplify life if there were just
    one default temp directory, $PGDATA/base/pgsql_tmp/, plus one per
    configured temp tablespace, $PGDATA/pg_tblspc/NNNN/pgsql_tmp/.
    what the NNNN directory shoud be, i understand ypur idea as just
    $PGDATA/pg_tblspc/pgsql_tmp/...
    Am I right in thinking we didn't get an updated patch in yet?

    Any help needed here?

    This seems a very important patch for manageability and it would be a
    shame to miss out on it for 8.3 since its been a TODO item for so long.

    --
    Simon Riggs
    EnterpriseDB http://www.enterprisedb.com
    --
    Bruce Momjian <bruce@momjian.us> http://momjian.us
    EnterpriseDB http://www.enterprisedb.com

    + If your life is a hard drive, Christ can be your backup. +

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedMar 5, '07 at 10:45p
activeApr 2, '07 at 5:55p
posts6
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase