[WIP] GUC for temp_tablespaces

On 3/17/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Jaime Casanova" <systemguards(at)gmail(dot)com> writes:
> > On 3/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.
>

ok. i have done that.
I know this is not the time i told you but i was busy at job.

i haven't did anything about RemovePgTempFiles() yet, because i want
to know of the posibility of getting this on 8.3

--
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

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

Jaime Casanova wrote:
> On 3/17/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > "Jaime Casanova" <systemguards(at)gmail(dot)com> writes:
> > > On 3/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.
> >
>
> ok. i have done that.
> I know this is not the time i told you but i was busy at job.
>
> i haven't did anything about RemovePgTempFiles() yet, because i want
> to know of the posibility of getting this on 8.3
>
> --
> 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

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

On 5/3/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Your patch has been added to the PostgreSQL unapplied patches list at:
>

This is an updated version of the patch.

Tom objections:
- fd.c is too low level for calling code from commands/tablespace.c.
This was fixed adding a second parameter to BufFileCreateTemp() to send
the tblspcOid (this function is called from executor/nodeHashJoin.c,
utils/sort/logtape.c and utils/sort/tuplestore.c). Are these places ok?

- RemovePgTempFilesInDir() has no support for removing temp files from
strange locations.
Per Tom suggestion temp files are now created in: base/pgsql_tmp and
pg_tblspc/$oid_tblspc/pgsql_tmp. So i just refactor RemovePgTempFiles()
to call RemovePgTempFilesInDir() with base and pg_tblspc/$oid_tblspc's
pgsql_tmp

Other changes in code:
fd.c:
functions make_database_relative() and FileNameOpenFile() were marked
as NOT_USED. objections to simply delete them?
also added OpenTempFileInTblspc() to create the tempfilepath and call
to PathNameOpenFile()
buffile.c:
also added a new tblspcOid field to BufFile struct to use it in extendBufFile()

Problems:
While the patch passes all the regression tests i still have a problem
when doin this:

sgerp=# set temp_tablespaces = '';
ERROR: tablespace "" does not exist

note that setting temp_tablespaces = '' from postgresql.conf works well.

maybe this is silly but it's too late for me... i will keep trying
tomorrow unless someone else has fixed it.

comments?

--
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

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

Jaime Casanova wrote:
> On 5/3/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >
> > Your patch has been added to the PostgreSQL unapplied patches list at:
> >
>
> This is an updated version of the patch.
>
> Tom objections:
> - fd.c is too low level for calling code from commands/tablespace.c.
> This was fixed adding a second parameter to BufFileCreateTemp() to send
> the tblspcOid (this function is called from executor/nodeHashJoin.c,
> utils/sort/logtape.c and utils/sort/tuplestore.c). Are these places ok?
>
> - RemovePgTempFilesInDir() has no support for removing temp files from
> strange locations.
> Per Tom suggestion temp files are now created in: base/pgsql_tmp and
> pg_tblspc/$oid_tblspc/pgsql_tmp. So i just refactor RemovePgTempFiles()
> to call RemovePgTempFilesInDir() with base and pg_tblspc/$oid_tblspc's
> pgsql_tmp
>
> Other changes in code:
> fd.c:
> functions make_database_relative() and FileNameOpenFile() were marked
> as NOT_USED. objections to simply delete them?
> also added OpenTempFileInTblspc() to create the tempfilepath and call
> to PathNameOpenFile()
> buffile.c:
> also added a new tblspcOid field to BufFile struct to use it in extendBufFile()
>
>
> Problems:
> While the patch passes all the regression tests i still have a problem
> when doin this:
>
> sgerp=# set temp_tablespaces = '';
> ERROR: tablespace "" does not exist
>
> note that setting temp_tablespaces = '' from postgresql.conf works well.
>
> maybe this is silly but it's too late for me... i will keep trying
> tomorrow unless someone else has fixed it.
>
> comments?
>
> --
> 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

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

On 5/5/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Your patch has been added to the PostgreSQL unapplied patches list at:

This is final version of the patch (i hope), at least it fixes the
problem i had yesterday.

--
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

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

Jaime Casanova wrote:
> On 5/5/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >
> > Your patch has been added to the PostgreSQL unapplied patches list at:
>
> This is final version of the patch (i hope), at least it fixes the
> problem i had yesterday.
>
> --
> 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

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

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

Am Samstag, 5. Mai 2007 16:40 schrieb Jaime Casanova:
> On 5/5/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Your patch has been added to the PostgreSQL unapplied patches list at:
>
> This is final version of the patch (i hope), at least it fixes the
> problem i had yesterday.

What I have been missing all along in these patches is an explanation for what
it means to list multiple temporary tablespaces. Are they used in order, or
the first one that exists, or what?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 5/8/07, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Am Samstag, 5. Mai 2007 16:40 schrieb Jaime Casanova:
> > On 5/5/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > > Your patch has been added to the PostgreSQL unapplied patches list at:
> >
> > This is final version of the patch (i hope), at least it fixes the
> > problem i had yesterday.
>
> What I have been missing all along in these patches is an explanation for what
> it means to list multiple temporary tablespaces. Are they used in order, or
> the first one that exists, or what?
>

http://archives.postgresql.org/pgsql-hackers/2007-01/msg00531.php
http://archives.postgresql.org/pgsql-patches/2007-01/msg00282.php

in src/backend/commands/tablespace.c:assign_temp_tablespaces():

/*
* Select the first tablespace to use
*/
Assert(num_temp_tablespaces >= 0);
if (num_temp_tablespaces != 0)
next_temp_tablespace = MyProcPid % num_temp_tablespaces;

--
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

Am Mittwoch, 9. Mai 2007 02:21 schrieb Jaime Casanova:
> > What I have been missing all along in these patches is an explanation for
> > what it means to list multiple temporary tablespaces. Are they used in
> > order, or the first one that exists, or what?
>
> http://archives.postgresql.org/pgsql-hackers/2007-01/msg00531.php
> http://archives.postgresql.org/pgsql-patches/2007-01/msg00282.php

Those are discussions of possible ideas, not an acceptable documentation of
this feature.

> in src/backend/commands/tablespace.c:assign_temp_tablespaces():

That also isn't an acceptable place to put feature documentation.

> /*
> * Select the first tablespace to use
> */
> Assert(num_temp_tablespaces >= 0);
> if (num_temp_tablespaces != 0)
> next_temp_tablespace = MyProcPid % num_temp_tablespaces;

What does this mean? Is there code that selects the second tablespace to use?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 5/9/07, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Am Mittwoch, 9. Mai 2007 02:21 schrieb Jaime Casanova:
> > > What I have been missing all along in these patches is an explanation for
> > > what it means to list multiple temporary tablespaces. Are they used in
> > > order, or the first one that exists, or what?
> >
> > http://archives.postgresql.org/pgsql-hackers/2007-01/msg00531.php
> > http://archives.postgresql.org/pgsql-patches/2007-01/msg00282.php
>
> Those are discussions of possible ideas, not an acceptable documentation of
> this feature.
>

ahh... ok, obviously a misunderstood you... what you were asking for
is user visible documentation, isn't it?

what the patch does is to select the first tablespace from the list
pseudo-randomicaly (MyProcPid % num_temp_tablespaces) and then cycle
in order through the list every time we call GetTempTablespace().
Every backend will start (hopefully) in a different tablespace and
will keep its own iterator for the list.
A BufFile will use the same tablespace for every file it has.
If we can't create the file in the selected tablespace we fall into
$PGDATA/base/pgsql_tmp, now that i think on it we should be sending a
warning that the file couldn't be created.

About the docs, what about something along the lines, in config.sgml:
"The first tablespace that will be used is choosen randomly from the
list, starting from that with cycle through the list in order.

--
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