Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On May 27, 2007, at 1:19 PM, Robert Treat wrote: On Friday 25 May 2007 12:39, Jaime Casanova wrote: On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote: Bernd Helmle [EMAIL PROTECTED] writes: No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. I believe we do not call RemovePgTempFiles during a crash recovery cycle; this is intentional on the theory that the temp files might contain useful debugging clues. So there is a potential problem there. Not sure how important it really is though --- neither crashes nor tablespace drops ought to be so common that we need a really nice solution. While tablespace operations won't normally be a day-to-day thing, it's not good if users can't delete a tablespace because it's not empty, but can't find out what exactly is in the tablespace. the only semi-sane solution i can think of, is to have a superuser only function that acts as a wrapper for RemovePgTempFiles(), but still exists a chance for shoot yourself on the foot... If there was a way for DBA's to know they could safely delete the left-over files (maybe the files timestamp is older than postmaster start; though not sure how you measure that), then I think this would be enough to give them a way out. Of course maybe that level of smarts could be put into drop tablespace itself? If we're saving data on crashes with the intention that it might be useful then we need to be able to get a list of what those files are, and possibly be able to delete them easily. -- Jim Nasby[EMAIL PROTECTED] EnterpriseDB http://enterprisedb.com 512.569.9461 (cell) ---(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
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/27/07, Robert Treat [EMAIL PROTECTED] wrote: On Friday 25 May 2007 12:39, Jaime Casanova wrote: On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote: Bernd Helmle [EMAIL PROTECTED] writes: --On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova [EMAIL PROTECTED] wrote: No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. I believe we do not call RemovePgTempFiles during a crash recovery cycle; this is intentional on the theory that the temp files might contain useful debugging clues. ah, i forgot that So there is a potential problem there. Not sure how important it really is though --- neither crashes nor tablespace drops ought to be so common that we need a really nice solution. the only semi-sane solution i can think of, is to have a superuser only function that acts as a wrapper for RemovePgTempFiles(), but still exists a chance for shoot yourself on the foot... If there was a way for DBA's to know they could safely delete the left-over files (maybe the files timestamp is older than postmaster start; though not sure how you measure that), then I think this would be enough to give them a way out. Of course maybe that level of smarts could be put into drop tablespace itself? i don't think silently delete the files is a good idea, specially if the files are left there intencionally... but what, exactly, we want to do? delete the files or maybe sending an HINT just after the error so we can inform the DBA about the temp files and let him decide. comments? BTW, postmaster startup is in PgStartTime, right? -- 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 ---(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
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On Tuesday 29 May 2007 20:06, Jaime Casanova wrote:
On 5/27/07, Robert Treat [EMAIL PROTECTED] wrote:
On Friday 25 May 2007 12:39, Jaime Casanova wrote:
On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote:
Bernd Helmle [EMAIL PROTECTED] writes:
--On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova
[EMAIL PROTECTED] wrote:
No, because the RemovePgTempFiles() call in PostmasterMain() will
remove all tmp files at startup.
I believe we do not call RemovePgTempFiles during a crash recovery
cycle; this is intentional on the theory that the temp files might
contain useful debugging clues.
ah, i forgot that
So there is a potential problem there.
Not sure how important it really is though --- neither crashes nor
tablespace drops ought to be so common that we need a really nice
solution.
the only semi-sane solution i can think of, is to have a superuser
only function that acts as a wrapper for RemovePgTempFiles(), but
still exists a chance for shoot yourself on the foot...
If there was a way for DBA's to know they could safely delete the
left-over files (maybe the files timestamp is older than postmaster
start; though not sure how you measure that), then I think this would be
enough to give them a way out. Of course maybe that level of smarts
could be put into drop tablespace itself?
i don't think silently delete the files is a good idea, specially if
the files are left there intencionally...
but what, exactly, we want to do? delete the files or maybe sending an
HINT just after the error so we can inform the DBA about the temp
files and let him decide.
I guess the thing to do is error that you cannot drop a non-empty tablespace,
with a hint that there are files older than pg start time in the tablespace
directory.
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
---(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
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On Friday 25 May 2007 12:39, Jaime Casanova wrote:
On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote:
Bernd Helmle [EMAIL PROTECTED] writes:
--On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova
[EMAIL PROTECTED] wrote:
No, because the RemovePgTempFiles() call in PostmasterMain() will
remove all tmp files at startup.
I believe we do not call RemovePgTempFiles during a crash recovery
cycle; this is intentional on the theory that the temp files might
contain useful debugging clues.
ah, i forgot that
So there is a potential problem there.
Not sure how important it really is though --- neither crashes nor
tablespace drops ought to be so common that we need a really nice
solution.
the only semi-sane solution i can think of, is to have a superuser
only function that acts as a wrapper for RemovePgTempFiles(), but
still exists a chance for shoot yourself on the foot...
If there was a way for DBA's to know they could safely delete the left-over
files (maybe the files timestamp is older than postmaster start; though not
sure how you measure that), then I think this would be enough to give them a
way out. Of course maybe that level of smarts could be put into drop
tablespace itself?
--
Robert Treat
Build A Brighter LAMP :: Linux Apache {middleware} PostgreSQL
---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/25/07, Bernd Helmle [EMAIL PROTECTED] wrote: --On Freitag, Mai 25, 2007 00:02:06 + Jaime Casanova [EMAIL PROTECTED] wrote: sounds good. can we see the new patch? Attached tablespace.c.diff shows my current changes to use an OID lookup list. on second thought, what happens if someone drops an empty tablespace, that already is in the temp_tablespace GUC, and recreate it (one scenario for this is if you want to move the tablespace to a newer better/faster location). then will have an invalid oid until at least you execute a new SET temp_tablespaces. And we know some DBA's doesn't read the manual, so maybe this behaviour will be unexpected for them... the reason for those messages is that the tablespace can get full or can be dropped before use, so we throw the message for the dba to take actions. if no one thinks is a good idea the message can be removed. I could imagine that this could irritate DBA's (at least, that is what happened to me during testing). Well at least with this message they will be alerted, but still seems silly to me... (make a SET with the same list just for updating cached OID's) -- 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote: Jaime Casanova [EMAIL PROTECTED] writes: On 5/24/07, Alvaro Herrera [EMAIL PROTECTED] wrote: What happens if you create a cursor that stores something (sort intermediate results?) in a temp tablespace, FETCH some from it, then someone else drops the tablespace and FETCH some more? you can't drop a tablespace that is not empty. So a temp file left over by a crashed backend would indefinitely prevent dropping the tablespace, until someone manually cleaned it up? No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. -- 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
--On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova [EMAIL PROTECTED] wrote: No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. Hmm isn't RemovePgTempFiles() called on postmaster startup only? What will happen if a temp tablespace is out of disk space, and the backend leaves all previously created temp files there? Under these assumption we'll need to restart the postmaster to get a clean tablespace ready to drop... -- Thanks Bernd ---(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
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
Bernd Helmle [EMAIL PROTECTED] writes: --On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova [EMAIL PROTECTED] wrote: No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. Hmm isn't RemovePgTempFiles() called on postmaster startup only? What will happen if a temp tablespace is out of disk space, and the backend leaves all previously created temp files there? Under these assumption we'll need to restart the postmaster to get a clean tablespace ready to drop... Theoretically, a backend will always remove its temp files during transaction abort, so the only case that is really of concern is a backend crashing before it can get around to doing that. However, I believe we do not call RemovePgTempFiles during a crash recovery cycle; this is intentional on the theory that the temp files might contain useful debugging clues. So there is a potential problem there. Not sure how important it really is though --- neither crashes nor tablespace drops ought to be so common that we need a really nice solution. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
--On Freitag, Mai 25, 2007 00:02:06 + Jaime Casanova
[EMAIL PROTECTED] wrote:
the original patch is from Albert Cervera =)
Ah, missed that, thanks ;)
sounds good. can we see the new patch?
Attached tablespace.c.diff shows my current changes to use an OID lookup
list.
the reason for those messages is that the tablespace can get full or
can be dropped before use, so we throw the message for the dba to take
actions. if no one thinks is a good idea the message can be removed.
I could imagine that this could irritate DBA's (at least, that is what
happened to me during testing). It's okay that someone could drop a
tablespace concurrently to other transactions, but i have concerns that
with temp_tablespaces this could happen during _queries_. Do queries
delete/recreate temp files during execution, maybe within sorts so that the
used temp tablespace looks empty for a certain period of time?
The silent
mechanism to drop a tablespace during temporary usage makes me a little
bit uncomfortable about its robustness.
maybe using the list you put in TopMemoryContext we can deny the
ability to drop the tablespace until it's removed from the list of
temp tablespaces.
That would mean we have to share this information between backends. This
looks complicated since every user could have its own temp_tablespaces
GUC
--
Thanks
BerndIndex: tablespace.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablespace.c,v
retrieving revision 1.45
diff -c -B -r1.45 tablespace.c
*** tablespace.c 22 Mar 2007 19:51:44 - 1.45
--- tablespace.c 25 May 2007 15:27:23 -
***
*** 63,73
#include utils/fmgroids.h
#include utils/guc.h
#include utils/lsyscache.h
! /* GUC variable */
char *default_tablespace = NULL;
static bool remove_tablespace_directories(Oid tablespaceoid, bool redo);
static void set_short_version(const char *path);
--- 63,80
#include utils/fmgroids.h
#include utils/guc.h
#include utils/lsyscache.h
+ #include utils/memutils.h
! /* GUC variables */
char *default_tablespace = NULL;
+ char *temp_tablespaces = NULL;
+ static int next_temp_tablespace;
+ static int num_temp_tablespaces;
+
+ /* OID list of current temp tablespaces */
+ static List *tmp_tblspc_lookup = NIL;
static bool remove_tablespace_directories(Oid tablespaceoid, bool redo);
static void set_short_version(const char *path);
***
*** 935,940
--- 942,1080
return result;
}
+ /*
+ * Routines for handling the GUC variable 'temp_tablespaces'.
+ */
+
+ /* assign_hook: validate new temp_tablespaces, do extra actions as needed */
+ const char *
+ assign_temp_tablespaces(const char *newval, bool doit, GucSource source)
+ {
+ char *rawname;
+ ListCell *l;
+ MemoryContext cur_cntxt;
+ List *namelist = NIL;
+ List *oidlist = NIL;
+
+ /* Need a modifiable copy of string */
+ rawname = pstrdup(newval);
+
+ /* Parse string into list of identifiers */
+ if (!SplitIdentifierString(rawname, ',', namelist))
+ {
+ /* syntax error in name list */
+ pfree(rawname);
+ list_free(namelist);
+ return NULL;
+ }
+
+ num_temp_tablespaces = 0;
+
+ foreach(l, namelist)
+ {
+ char *curname = (char *) lfirst(l);
+ if (curname[0] == '\0')
+ continue;
+
+ /*
+ * If we aren't inside a transaction, we cannot do database access so
+ * cannot verify the individual names. Must accept the list on faith.
+ */
+ if (source = PGC_S_INTERACTIVE IsTransactionState())
+ {
+ /*
+ * Verify that all the names are valid tablespace names
+ * We do not check for USAGE rights should we?
+ */
+ Oid cur_tblspc = get_tablespace_oid(curname);
+ if (cur_tblspc == InvalidOid)
+ {
+ ereport((source == PGC_S_TEST) ? NOTICE : ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(tablespace \%s\ does not exist, curname)));
+ }
+ else
+ {
+ /*
+ * Append new OID to temporary list. We can't
+ * use the lookup table directly, because there could
+ * be an ereport() in subsequent loops.
+ */
+ oidlist = lappend_oid(oidlist, cur_tblspc);
+ }
+ }
+ num_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;
+
+ /* Looks good for now, free any old lookup table and copy new OID
+ list to our lookup table in permanent storage */
+ cur_cntxt = MemoryContextSwitchTo(TopMemoryContext);
+
+ if (tmp_tblspc_lookup != NIL)
+ {
+ list_free(tmp_tblspc_lookup);
+ }
+
+ tmp_tblspc_lookup = list_copy(oidlist);
+ MemoryContextSwitchTo(cur_cntxt);
+
+ pfree(rawname);
+ list_free(namelist);
+ return newval;
+ }
+
+ /*
+ * GetTempTablespace -- get the OID of the
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/25/07, Tom Lane [EMAIL PROTECTED] wrote: Bernd Helmle [EMAIL PROTECTED] writes: --On Freitag, Mai 25, 2007 10:49:29 + Jaime Casanova [EMAIL PROTECTED] wrote: No, because the RemovePgTempFiles() call in PostmasterMain() will remove all tmp files at startup. I believe we do not call RemovePgTempFiles during a crash recovery cycle; this is intentional on the theory that the temp files might contain useful debugging clues. ah, i forgot that So there is a potential problem there. Not sure how important it really is though --- neither crashes nor tablespace drops ought to be so common that we need a really nice solution. the only semi-sane solution i can think of, is to have a superuser only function that acts as a wrapper for RemovePgTempFiles(), but still exists a chance for shoot yourself on the foot... -- 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/25/07, Bernd Helmle [EMAIL PROTECTED] wrote:
--On Freitag, Mai 25, 2007 00:02:06 + Jaime Casanova
[EMAIL PROTECTED] wrote:
sounds good. can we see the new patch?
Attached tablespace.c.diff shows my current changes to use an OID lookup
list.
+ if (source = PGC_S_INTERACTIVE IsTransactionState())
+ {
+ /*
+* Verify that all the names are valid tablespace names
+* We do not check for USAGE rights should we?
+*/
+ Oid cur_tblspc = get_tablespace_oid(curname);
+ if (cur_tblspc == InvalidOid)
+ {
+ ereport((source == PGC_S_TEST) ? NOTICE : ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+errmsg(tablespace \%s\ does not exist, curname)));
+ }
+ else
+ {
+ /*
+* Append new OID to temporary list. We can't
+* use the lookup table directly, because there could
+* be an ereport() in subsequent loops.
+*/
+ oidlist = lappend_oid(oidlist, cur_tblspc);
+ }
+ }
the list of oid's is only filled when you execute
SET temp_tablespaces = 'somelist'
but if you use the GUC in postgresql.conf at startup then not, so the
temp_tablespaces are not used even if they are setted
can you do that outside
+ if (source = PGC_S_INTERACTIVE IsTransactionState())
--
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
---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings
[HACKERS] Reviewing temp_tablespaces GUC patch
While looking at Jaime's last temp_tablespaces GUC patch, i've got some concerns about it's current implementation: 1) The code claims that OIDs of temp tablespaces couldn't be cached, therefore it is parsing and re-reading the GUCs in GetTempTablespace() with SplitIdentifierNames() over and over again. Because GetTempTablespace() is likely to be called many times in queries with a good amount of search operations, i believe this could be done better by allocating a list of OIDs in permanent storage (TopMemoryContext) and use this OID list to re-check them in GetTempTablespace() (i have modified the patch and it seems to work). This would save us to split the GUC every time. 2) It's possible that someone could drop a temporary tablespace between subsequent usage of GetTempTablespace() when they are empty. This leads to strange NOTICEs like NOTICE: could not create temporary file pg_tblspc/16387/pgsql_tmp/pgsql_tmp19942.0 during query execution. However, the code is save enough and switches back to base/pgsql_tmp then, but this looks a little bit ugly to me. The silent mechanism to drop a tablespace during temporary usage makes me a little bit uncomfortable about its robustness. Comments? -- Thanks Bernd ---(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
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
Bernd Helmle escribió: It's possible that someone could drop a temporary tablespace between subsequent usage of GetTempTablespace() when they are empty. This leads to strange NOTICEs like NOTICE: could not create temporary file pg_tblspc/16387/pgsql_tmp/pgsql_tmp19942.0 during query execution. However, the code is save enough and switches back to base/pgsql_tmp then, but this looks a little bit ugly to me. The silent mechanism to drop a tablespace during temporary usage makes me a little bit uncomfortable about its robustness. What happens if you create a cursor that stores something (sort intermediate results?) in a temp tablespace, FETCH some from it, then someone else drops the tablespace and FETCH some more? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/24/07, Bernd Helmle [EMAIL PROTECTED] wrote: While looking at Jaime's last temp_tablespaces GUC patch, i've got some concerns about it's current implementation: the original patch is from Albert Cervera =) 1) The code claims that OIDs of temp tablespaces couldn't be cached, therefore it is parsing and re-reading the GUCs in GetTempTablespace() with SplitIdentifierNames() over and over again. Because GetTempTablespace() is likely to be called many times in queries with a good amount of search operations, i believe this could be done better by allocating a list of OIDs in permanent storage (TopMemoryContext) and use this OID list to re-check them in GetTempTablespace() (i have modified the patch and it seems to work). This would save us to split the GUC every time. sounds good. can we see the new patch? 2) It's possible that someone could drop a temporary tablespace between subsequent usage of GetTempTablespace() when they are empty. This leads to strange NOTICEs like NOTICE: could not create temporary file pg_tblspc/16387/pgsql_tmp/pgsql_tmp19942.0 during query execution. However, the code is save enough and switches back to base/pgsql_tmp then, but this looks a little bit ugly to me. the reason for those messages is that the tablespace can get full or can be dropped before use, so we throw the message for the dba to take actions. if no one thinks is a good idea the message can be removed. The silent mechanism to drop a tablespace during temporary usage makes me a little bit uncomfortable about its robustness. maybe using the list you put in TopMemoryContext we can deny the ability to drop the tablespace until it's removed from the list of 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
On 5/24/07, Alvaro Herrera [EMAIL PROTECTED] wrote: Bernd Helmle escribió: It's possible that someone could drop a temporary tablespace between subsequent usage of GetTempTablespace() when they are empty. This leads to strange NOTICEs like NOTICE: could not create temporary file pg_tblspc/16387/pgsql_tmp/pgsql_tmp19942.0 during query execution. However, the code is save enough and switches back to base/pgsql_tmp then, but this looks a little bit ugly to me. The silent mechanism to drop a tablespace during temporary usage makes me a little bit uncomfortable about its robustness. What happens if you create a cursor that stores something (sort intermediate results?) in a temp tablespace, FETCH some from it, then someone else drops the tablespace and FETCH some more? you can't drop a tablespace that is not empty. -- 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 ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] Reviewing temp_tablespaces GUC patch
Jaime Casanova [EMAIL PROTECTED] writes: On 5/24/07, Alvaro Herrera [EMAIL PROTECTED] wrote: What happens if you create a cursor that stores something (sort intermediate results?) in a temp tablespace, FETCH some from it, then someone else drops the tablespace and FETCH some more? you can't drop a tablespace that is not empty. So a temp file left over by a crashed backend would indefinitely prevent dropping the tablespace, until someone manually cleaned it up? regards, tom lane ---(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
