Tablespace for temporary objects and sort files

On Wed, 2006-10-25 at 00:45 +0200, Albert Cervera Areny wrote:

Hope the diff and idents are ok.

Patches should be submitted in context diff (diff -c) format.

-Neil

On 10/24/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

Hi,

I'm trying to introduce myself into postgresql development and I'm working on
the "tablespace for temporary objects and sort files" TODO item.

some comments from a non-hacker:

your patch isn't doing nothing at all for temporary indexes... a quick
search for GetDefaultTablespace() shows this places...

postgres@casanova:~/PG_RELEASES/pgsql$ grep -lR GetDefaultTablespace *
src/backend/commands/indexcmds.c
src/backend/commands/tablecmds.c
src/backend/commands/tablespace.c
src/backend/executor/execMain.c
src/include/commands/tablespace.h

Now a question, why not using the same GetDefaultTablespace() with an
argument indicating if the object is temporary, if it is get the
default tablespace for temp objects else get the default tablespace
for permanent object... just an idea...

How can I test that the tablespace is correctly used for sort files? Is there
an easy way? Or should I reduce work_mem to a minimum, populate the database
with data and try an "ORDER BY"?

yes, that seems to work... i reduce, just in case, work_mem,
shared_buffers and temp_buffers...

Now, PG_TEMP_FILES_DIR seems to add just pgsql_temp to the filename. i
think you the function you have to modify here is
make_database_relative() that adds base/#database_oid# at the
beginning of the path of the file.

Hope the diff and idents are ok. Please let me know if there's something wrong
with them.

diff -c is the way

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

Sorry about the wrong diff format. Attached is the good one.

A Dimecres 25 Octubre 2006 09:07, Neil Conway va escriure:

This has been saved for the 8.3 release:

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

---------------------------------------------------------------------------
Albert Cervera Areny wrote:

Sorry about the wrong diff format. Attached is the good one.

A Dimecres 25 Octubre 2006 09:07, Neil Conway va escriure:

On Wed, 2006-10-25 at 00:45 +0200, Albert Cervera Areny wrote:

Hope the diff and idents are ok.

Patches should be submitted in context diff (diff -c) format.

-Neil

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf Of Bruce Momjian
Sent: 26 October 2006 14:19
To: Albert Cervera Areny
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Tablespace for temporary objects and sort files

This has been saved for the 8.3 release:

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

I believe Albert was looking for feedback at this stage, rather than
submitting a completed patch.

http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

Regards, Dave

Dave Page wrote:

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf Of Bruce Momjian
Sent: 26 October 2006 14:19
To: Albert Cervera Areny
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Tablespace for temporary objects and sort files

This has been saved for the 8.3 release:

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

I believe Albert was looking for feedback at this stage, rather than
submitting a completed patch.

http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

Right, but it is being kept so that we can ping him when we start 8.3 to
get a more recent version.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

bruce wrote:

Dave Page wrote:

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf Of Bruce Momjian
Sent: 26 October 2006 14:19
To: Albert Cervera Areny
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Tablespace for temporary objects and sort files

This has been saved for the 8.3 release:

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

I believe Albert was looking for feedback at this stage, rather than
submitting a completed patch.

http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

Right, but it is being kept so that we can ping him when we start 8.3 to
get a more recent version.

Sorry, to clarify, I saved the entire thread where he stated he was
looking for feedback, so when we go back to it, we know it needs work.

--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com

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

-----Original Message-----
From: Bruce Momjian [mailto:bruce@momjian.us]
Sent: 26 October 2006 14:37
To: bruce@momjian.us
Cc: Dave Page; Albert Cervera Areny; pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Tablespace for temporary objects and sort files

bruce wrote:

Dave Page wrote:

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf

Of Bruce Momjian

Sent: 26 October 2006 14:19
To: Albert Cervera Areny
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Tablespace for temporary objects

and sort files

This has been saved for the 8.3 release:

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

I believe Albert was looking for feedback at this stage,

rather than

submitting a completed patch.

http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

Right, but it is being kept so that we can ping him when we

start 8.3 to

get a more recent version.

Sorry, to clarify, I saved the entire thread where he stated he was
looking for feedback, so when we go back to it, we know it needs work.

OK.

/D

On 10/25/06, Jaime Casanova <systemguards@gmail.com> wrote:

On 10/24/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

I'm trying to introduce myself into postgresql development and I'm working on
the "tablespace for temporary objects and sort files" TODO item.

Now, PG_TEMP_FILES_DIR seems to add just pgsql_temp to the filename. i
think you the function you have to modify here is
make_database_relative() that adds base/#database_oid# at the
beginning of the path of the file.

some tests shows that your patch is doing the wrong thing for temp
files, actually the server is crashing

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

On 10/24/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

Hi,

I'm trying to introduce myself into postgresql development and I'm working on
the "tablespace for temporary objects and sort files" TODO item. The attached
patch shows what I've already done. The GUC is currently
called "temp_tablespaces".

hi albert. are you working on this?

i'm willing to help... if you are not working on this, i will make a try...

comments on the original patch?
AFAIR, it fails the regrress tests when executing initdb...
more here http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

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

On 12/24/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

Hi,
yes I'm working on it. I've got a more recent version that doesn't crash on
initdb and works with sort files. I've got a couple of things pending and
will post it as soon as I can. If you want I could make a diff and send it to
you in case you wanted to test/improve the patch.
Refering to some of the comment I never answered (sorry about that) I prefer
to use GetTempTablespace() as I think it's easier to read/understand than
GetDefaultTablespace(true).
And thanks for pointing to temporary indexes. There seems not to be temporary
indexes but indexes of temporary tables, which could use GetTempTablespace()
too...

yeah, it was late and i was almost asleep... i was thinking in
temporary sequences, but indexes on temporary tables it's not a bad
idea too...
that was the reason i think it's better to use the same
GetDefaultTablespace() function it's less intrussive and is not
directed to one particular object but all temp objects can benefit...

i will wait your patch when you think is ready for discussion...

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

Hi,
here's a new version of the patch against HEAD with both, table and sort
files working correctly for me. Regression tests work too.
I'd like to ask again the question I made on the first post as no answer was
given at that time:

"The GetTempTablespace function correctly returns a different tablespace
each time is called, but I store the position of the last tablespace used
with an integer and iterate through the list of tablespaces each time. I
tried to keep the iterator from call to call but I got a segfault, I
imagine due to the memory context. Should I try to keep the iterator? How
can I do it?"

Now I'm working on some regression tests that could be added to
tablespace.source using something like:

SET temp_tablespaces='testspace';

CREATE TEMP TABLE temp_foo(a VARCHAR);

SELECT COUNT(pg_ls_dir('pg_tblspc/' || (SELECT oid FROM
pg_catalog.pg_tablespace WHERE spcname='testspace' .... )

Do you think it's a good idea to list the files in the directory of the
tablespace to ensure temporary table files are created where they should? Do
you think there is a smart way to ensure the same with sort files?

Any feedback welcome!

A Dimecres 25 Octubre 2006 00:45, Albert Cervera Areny va escriure:

On 12/27/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

Hi,
here's a new version of the patch against HEAD with both, table and sort
files working correctly for me. Regression tests work too.

ok, i will test it a little ... what about temp tables created by
SELECT INTO TEMP?
look at src/backend/executor/execMain.c:OpenIntoRel()

what about other temporary objects?

I'd like to ask again the question I made on the first post as no answer was
given at that time:

"The GetTempTablespace function correctly returns a different tablespace
each time is called, but I store the position of the last tablespace used
with an integer and iterate through the list of tablespaces each time. I
tried to keep the iterator from call to call but I got a segfault, I
imagine due to the memory context. Should I try to keep the iterator? How
can I do it?"

i didn't read this last time, i will take a look at it now... when you
post for the first time hackers where busy releasing 8.2.0, maybe they
will pay more atention now :)

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

On 12/27/06, Albert Cervera Areny <albertca@hotpop.com> wrote:

Hi,
here's a new version of the patch against HEAD with both, table and sort
files working correctly for me. Regression tests work too.
I'd like to ask again the question I made on the first post as no answer was
given at that time:

"The GetTempTablespace function correctly returns a different tablespace
each time is called, but I store the position of the last tablespace used
with an integer and iterate through the list of tablespaces each time. I
tried to keep the iterator from call to call but I got a segfault, I
imagine due to the memory context. Should I try to keep the iterator? How
can I do it?"

Now I'm working on some regression tests that could be added to
tablespace.source using something like:

SET temp_tablespaces='testspace';

CREATE TEMP TABLE temp_foo(a VARCHAR);

SELECT COUNT(pg_ls_dir('pg_tblspc/' || (SELECT oid FROM
pg_catalog.pg_tablespace WHERE spcname='testspace' .... )

seems is working fine... i actually looked for the files in the
tablespace directory...
have you looked my past 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

Here's a new version that takes into account the SELECT INTO TEMP case. Thanks
Jaime!

What other temporary objects do you think should be considered?

Any other comments on the patch?

A Dijous 04 Gener 2007 05:33, Jaime Casanova va escriure:

On 1/5/07, Albert Cervera Areny <albertca@hotpop.com> wrote:

Here's a new version that takes into account the SELECT INTO TEMP case. Thanks
Jaime!

ok. seems good to me...

What other temporary objects do you think should be considered?

sorry for the noise, seems i'm thinking on other dbms... i really
thought sequences use tablespaces but it seems i was wrong...

maybe once this patch is applied you can think on make indexes and
[temp] sequences on temp tables use the same temp_tablespace that
table is using...

Any other comments on the patch?

yes. there are 2 things missing:

the first one is put temp_tablespaces in postgresql.conf and/or making
it a per database parameter (i don't think it should be a per user
parameter)

the second are docs... ;)

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

On 1/8/07, Jaime Casanova <systemguards@gmail.com> wrote:

maybe once this patch is applied you can think on make indexes and
[temp] sequences on temp tables use the same temp_tablespace that
table is using...

actually, the index part is easy... do you want to do it?

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

I don't have much time lately so if you're willing to work on it, please do.

A Dimarts 09 Gener 2007 02:51, Jaime Casanova va escriure:

On 1/9/07, Albert Cervera Areny <albertca@hotpop.com> wrote:

I don't have much time lately so if you're willing to work on it, please do.

after doing the index part i revisited the patch again and saw that
there is something fundamentally wrong here (sorry for no noticing
that before :( )

your are using local backend variables for the iterator so two
different backends will use to different variables... because of that
if you have 3 temp tablespaces in your temp_tablespaces guc and start
100 backend and every one of them create just one temp table those 100
temp tables will be in the first temp tablespace... :(

i will try to fix that as well... unless you want to do it, just tell me...

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