pgsql: Add GUC temp_tablespaces to provide a default location for

Started by Nonamealmost 19 years ago14 messages
#1Noname
momjian@postgresql.org

Log Message:
-----------
Add GUC temp_tablespaces to provide a default location for temporary
objects.

Jaime Casanova

Modified Files:
--------------
pgsql/doc/src/sgml:
config.sgml (r1.104 -> r1.105)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/doc/src/sgml/config.sgml.diff?r1=1.104&r2=1.105)
pgsql/src/backend/commands:
indexcmds.c (r1.153 -> r1.154)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/indexcmds.c.diff?r1=1.153&r2=1.154)
tablecmds.c (r1.211 -> r1.212)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/tablecmds.c.diff?r1=1.211&r2=1.212)
tablespace.c (r1.40 -> r1.41)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/tablespace.c.diff?r1=1.40&r2=1.41)
pgsql/src/backend/executor:
execMain.c (r1.284 -> r1.285)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/execMain.c.diff?r1=1.284&r2=1.285)
pgsql/src/backend/storage/file:
fd.c (r1.134 -> r1.135)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/storage/file/fd.c.diff?r1=1.134&r2=1.135)
pgsql/src/backend/utils/misc:
guc.c (r1.369 -> r1.370)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/guc.c.diff?r1=1.369&r2=1.370)
postgresql.conf.sample (r1.204 -> r1.205)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/misc/postgresql.conf.sample.diff?r1=1.204&r2=1.205)
pgsql/src/include/commands:
tablespace.h (r1.14 -> r1.15)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/commands/tablespace.h.diff?r1=1.14&r2=1.15)
pgsql/src/include/utils:
guc.h (r1.78 -> r1.79)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/guc.h.diff?r1=1.78&r2=1.79)

#2Jaime Casanova
systemguards@gmail.com
In reply to: Noname (#1)
Re: pgsql: Add GUC temp_tablespaces to provide a default location for

On 1/24/07, Bruce Momjian <momjian@postgresql.org> wrote:

Log Message:
-----------
Add GUC temp_tablespaces to provide a default location for temporary
objects.

Jaime Casanova

the credit is for Albert Cervera Areny, I just did some minor
revisions and documentation:
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

#3Bruce Momjian
bruce@momjian.us
In reply to: Jaime Casanova (#2)
Re: pgsql: Add GUC temp_tablespaces to provide a

Jaime Casanova wrote:

On 1/24/07, Bruce Momjian <momjian@postgresql.org> wrote:

Log Message:
-----------
Add GUC temp_tablespaces to provide a default location for temporary
objects.

Jaime Casanova

the credit is for Albert Cervera Areny, I just did some minor
revisions and documentation:
http://archives.postgresql.org/pgsql-patches/2006-10/msg00141.php

CVS commit message added so release notes will have proper attribution.

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

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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Noname (#1)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

momjian@postgresql.org (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

#5Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#4)
1 attachment(s)
Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

OK, patch reverted. Authors, would you please resubmit with fixes?
Thanks.

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

Tom Lane wrote:

momjian@postgresql.org (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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

Attachments:

/rtmp/difftext/x-diffDownload
Index: doc/src/sgml/config.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/config.sgml,v
retrieving revision 1.104
retrieving revision 1.105
diff -c -r1.104 -r1.105
*** doc/src/sgml/config.sgml	20 Jan 2007 21:30:26 -0000	1.104
--- doc/src/sgml/config.sgml	25 Jan 2007 04:35:10 -0000	1.105
***************
*** 3398,3403 ****
--- 3398,3432 ----
        </listitem>
       </varlistentry>
  
+      <varlistentry id="guc-temp-tablespaces" xreflabel="temp_tablespaces">
+       <term><varname>temp_tablespaces</varname> (<type>string</type>)</term>
+       <indexterm>
+        <primary><varname>temp_tablespaces</> configuration parameter</primary>
+       </indexterm>
+       <indexterm><primary>tablespace</><secondary>temp</></>
+       <listitem>
+        <para>
+         This variable specifies tablespaces in which to create temp
+         objects (temp tables and indexes on temp tables) when a 
+         <command>CREATE</> command does not explicitly specify a tablespace 
+         and temp files when necessary (eg. for sorting operations).
+        </para>
+ 
+        <para>
+         The value is either a list of names of tablespaces, or an empty 
+         string to specify using the default tablespace of the current database.
+         If the value does not match the name of any existing tablespace,
+         <productname>PostgreSQL</> will automatically use the default
+         tablespace of the current database.
+        </para>
+ 
+        <para>
+         For more information on tablespaces,
+         see <xref linkend="manage-ag-tablespaces">.
+        </para>
+       </listitem>
+      </varlistentry>
+ 
       <varlistentry id="guc-check-function-bodies" xreflabel="check_function_bodies">
        <term><varname>check_function_bodies</varname> (<type>boolean</type>)</term>
        <indexterm>
Index: src/backend/commands/indexcmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/indexcmds.c,v
retrieving revision 1.153
retrieving revision 1.154
diff -c -r1.153 -r1.154
*** src/backend/commands/indexcmds.c	20 Jan 2007 23:13:01 -0000	1.153
--- src/backend/commands/indexcmds.c	25 Jan 2007 04:35:10 -0000	1.154
***************
*** 209,215 ****
  	}
  	else
  	{
! 		tablespaceId = GetDefaultTablespace();
  		/* note InvalidOid is OK in this case */
  	}
  
--- 209,221 ----
  	}
  	else
  	{
!  		/*
!  		 * if the target table is temporary then use a temp_tablespace
!  		 */
!  		if (!rel->rd_istemp)
! 			tablespaceId = GetDefaultTablespace();
!  		else
!  			tablespaceId = GetTempTablespace();
  		/* note InvalidOid is OK in this case */
  	}
  
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.211
retrieving revision 1.212
diff -c -r1.211 -r1.212
*** src/backend/commands/tablecmds.c	25 Jan 2007 04:17:45 -0000	1.211
--- src/backend/commands/tablecmds.c	25 Jan 2007 04:35:10 -0000	1.212
***************
*** 334,339 ****
--- 334,343 ----
  					 errmsg("tablespace \"%s\" does not exist",
  							stmt->tablespacename)));
  	}
+ 	else if (stmt->relation->istemp)
+ 	{
+ 		tablespaceId = GetTempTablespace();
+ 	}
  	else
  	{
  		tablespaceId = GetDefaultTablespace();
Index: src/backend/commands/tablespace.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/tablespace.c,v
retrieving revision 1.40
retrieving revision 1.41
diff -c -r1.40 -r1.41
*** src/backend/commands/tablespace.c	5 Jan 2007 22:19:26 -0000	1.40
--- src/backend/commands/tablespace.c	25 Jan 2007 04:35:10 -0000	1.41
***************
*** 65,73 ****
  #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);
--- 65,76 ----
  #include "utils/lsyscache.h"
  
  
! /* GUC variables */
  char	   *default_tablespace = NULL;
+ char       *temp_tablespaces = NULL;
  
+ int	   next_temp_tablespace;
+ int	   num_temp_tablespaces;
  
  static bool remove_tablespace_directories(Oid tablespaceoid, bool redo);
  static void set_short_version(const char *path);
***************
*** 930,935 ****
--- 933,1074 ----
  	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;
+ 	List	   *namelist;
+ 	ListCell   *l;
+ 
+ 	/* 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 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 tablspace names 
+ 			 * We do not check for USAGE rights should we?
+ 			 */
+ 			if (get_tablespace_oid(curname) == InvalidOid)
+ 				ereport((source == PGC_S_TEST) ? NOTICE : ERROR,
+ 						(errcode(ERRCODE_UNDEFINED_OBJECT),
+ 						errmsg("tablespace \"%s\" does not exist", curname)));
+ 		}
+ 		num_temp_tablespaces++;
+ 	}
+ 
+ 	/*
+ 	 * Select the first tablespace to use
+ 	 */
+ 	next_temp_tablespace = MyProcPid % num_temp_tablespaces;
+ 
+ 	pfree(rawname);
+ 	list_free(namelist);
+ 	return newval;
+ }
+ 
+ /*
+  * GetTempTablespace -- get the OID of the tablespace for temporary objects
+  *
+  * May return InvalidOid to indicate "use the database's default tablespace"
+  *
+  * This exists to hide the temp_tablespace GUC variable.
+  */
+ Oid
+ GetTempTablespace(void)
+ {
+ 	Oid			result;
+ 	char *curname = NULL;
+ 	char *rawname;
+ 	List *namelist;
+ 	ListCell *l;
+ 	int i = 0;
+ 	
+ 	if ( temp_tablespaces == NULL )
+ 		return InvalidOid;
+ 
+ 	/* Need a modifiable version of temp_tablespaces */
+ 	rawname = pstrdup(temp_tablespaces);
+ 
+ 	/* Parse string into list of identifiers */
+ 	if (!SplitIdentifierString(rawname, ',', &namelist))
+ 	{
+ 		/* syntax error in name list */
+ 		pfree(rawname);
+ 		list_free(namelist);
+ 		return InvalidOid;
+ 	}
+ 
+ 	/* 
+ 	 * Iterate through the list of namespaces until the one we need 
+ 	 * (next_temp_tablespace) 
+ 	 */
+ 	foreach(l, namelist)
+ 	{
+ 		curname = (char *) lfirst(l);
+ 		if ( i == next_temp_tablespace )
+ 			break;
+ 		i++;
+ 	}
+ 
+ 
+ 	/* Prepare for the next time the function is called */
+ 	next_temp_tablespace++;
+ 	if (next_temp_tablespace == num_temp_tablespaces)
+ 		next_temp_tablespace = 0;
+ 
+ 	/* Fast path for temp_tablespaces == "" */
+ 	if ( curname == NULL || curname[0] == '\0') {
+ 		list_free(namelist);
+ 		pfree(rawname);
+ 		return InvalidOid;
+ 	}
+ 
+ 	/*
+ 	 * It is tempting to cache this lookup for more speed, but then we would
+ 	 * fail to detect the case where the tablespace was dropped since the GUC
+ 	 * variable was set.  Note also that we don't complain if the value fails
+ 	 * to refer to an existing tablespace; we just silently return InvalidOid,
+ 	 * causing the new object to be created in the database's tablespace.
+ 	 */
+ 	result = get_tablespace_oid(curname);
+ 
+ 	/* We don't free rawname before because curname points to a part of it */
+ 	pfree(rawname);
+ 
+ 	/*
+ 	 * Allow explicit specification of database's default tablespace in
+ 	 * default_tablespace without triggering permissions checks.
+ 	 */
+ 	if (result == MyDatabaseTableSpace)
+ 		result = InvalidOid;
+ 	
+ 	list_free(namelist);
+ 	return result;
+ }
  
  /*
   * get_tablespace_oid - given a tablespace name, look up the OID
Index: src/backend/executor/execMain.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/executor/execMain.c,v
retrieving revision 1.284
retrieving revision 1.285
diff -c -r1.284 -r1.285
*** src/backend/executor/execMain.c	25 Jan 2007 02:17:26 -0000	1.284
--- src/backend/executor/execMain.c	25 Jan 2007 04:35:10 -0000	1.285
***************
*** 2409,2414 ****
--- 2409,2418 ----
  					 errmsg("tablespace \"%s\" does not exist",
  							parseTree->intoTableSpaceName)));
  	}
+ 	else if (parseTree->into->istemp)
+ 	{
+ 		tablespaceId = GetTempTablespace();
+ 	}
  	else
  	{
  		tablespaceId = GetDefaultTablespace();
Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.134
retrieving revision 1.135
diff -c -r1.134 -r1.135
*** src/backend/storage/file/fd.c	9 Jan 2007 22:03:51 -0000	1.134
--- src/backend/storage/file/fd.c	25 Jan 2007 04:35:10 -0000	1.135
***************
*** 46,51 ****
--- 46,53 ----
  #include <unistd.h>
  #include <fcntl.h>
  
+ #include "commands/tablespace.h"
+ 
  #include "miscadmin.h"
  #include "access/xact.h"
  #include "storage/fd.h"
***************
*** 76,81 ****
--- 78,84 ----
   */
  #define FD_MINFREE				10
  
+ #define OIDCHARS        10                      /* max chars printed by %u */
  
  /*
   * A number of platforms allow individual processes to open many more files
***************
*** 880,892 ****
  {
  	char		tempfilepath[MAXPGPATH];
  	File		file;
  
  	/*
! 	 * Generate a tempfile name that should be unique within the current
! 	 * database instance.
  	 */
! 	snprintf(tempfilepath, sizeof(tempfilepath),
! 			 "%s/%s%d.%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX,
  			 MyProcPid, tempFileCounter++);
  
  	/*
--- 883,933 ----
  {
  	char		tempfilepath[MAXPGPATH];
  	File		file;
+ 	Oid		oid;
+ 	char		*path;
+ 	int		pathlen;
  
  	/*
! 	 * Take a look what should be the path of the temporary file
  	 */
! 	oid = GetTempTablespace();
! 	if (oid != InvalidOid)
! 	{
! 		/*
! 		 * As we got a valid tablespace, try to create the
! 		 * file there
! 		 */
! 
! 		pathlen = strlen("pg_tblspc/") + OIDCHARS + 1;
! 		path = (char *) palloc(pathlen);
! 		snprintf(path, pathlen, "pg_tblspc/%u", oid );
! 
! 		/*
! 		 * Generate a tempfile name that should be unique within the current
! 		 * database instance.
! 		 */
! 		snprintf(tempfilepath, sizeof(tempfilepath),
! 				 "%s/%s%d.%ld", path, PG_TEMP_FILE_PREFIX,
! 				 MyProcPid, tempFileCounter++);
! 		pfree(path);
! 		file = PathNameOpenFile(tempfilepath,
! 							O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
! 							0600);
! 	}
! 
! 	/*
! 	 * Create a normal temporary file if no tablespace returned or
! 	 * couldn't create the file in the tablespace "oid"
! 	 */
! 	if (oid == InvalidOid || file <= 0) 
! 	{
! 		path = PG_TEMP_FILES_DIR;
! 		/*
! 		 * Generate a tempfile name that should be unique within the current
! 		 * database instance.
! 		 */
! 		snprintf(tempfilepath, sizeof(tempfilepath),
! 				 "%s/%s%d.%ld", path, PG_TEMP_FILE_PREFIX,
  			 MyProcPid, tempFileCounter++);
  
  	/*
***************
*** 918,924 ****
  		if (file <= 0)
  			elog(ERROR, "could not create temporary file \"%s\": %m",
  				 tempfilepath);
! 	}
  
  	/* Mark it for deletion at close */
  	VfdCache[file].fdstate |= FD_TEMPORARY;
--- 959,966 ----
  		if (file <= 0)
  			elog(ERROR, "could not create temporary file \"%s\": %m",
  				 tempfilepath);
! 		}
!  	}
  
  	/* Mark it for deletion at close */
  	VfdCache[file].fdstate |= FD_TEMPORARY;
***************
*** 1292,1297 ****
--- 1334,1353 ----
  		errno = save_errno;
  	}
  
+ 	/*
+ 	 * TEMPORARY hack to log the Windows error code on fopen failures, in
+ 	 * hopes of diagnosing some hard-to-reproduce problems.
+ 	 */
+ #ifdef WIN32
+ 	{
+ 		int			save_errno = errno;
+ 
+ 		elog(LOG, "Windows fopen(\"%s\",\"%s\") failed: code %lu, errno %d",
+ 			 name, mode, GetLastError(), save_errno);
+ 		errno = save_errno;
+ 	}
+ #endif
+ 
  	return NULL;
  }
  
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.369
retrieving revision 1.370
diff -c -r1.369 -r1.370
*** src/backend/utils/misc/guc.c	19 Jan 2007 16:58:46 -0000	1.369
--- src/backend/utils/misc/guc.c	25 Jan 2007 04:35:11 -0000	1.370
***************
*** 99,104 ****
--- 99,105 ----
  extern int	CommitDelay;
  extern int	CommitSiblings;
  extern char *default_tablespace;
+ extern char *temp_tablespaces;
  extern bool fullPageWrites;
  
  #ifdef TRACE_SORT
***************
*** 2291,2296 ****
--- 2292,2307 ----
  		"base64", assign_xmlbinary, NULL
  	},
  
+ 	{
+ 		{"temp_tablespaces", PGC_USERSET, PGC_S_FILE,
+ 			gettext_noop("Sets the tablespaces suitable for creating new objects and sort files."),
+ 			NULL,
+ 			GUC_LIST_INPUT | GUC_LIST_QUOTE 
+ 		},
+ 		&temp_tablespaces,
+ 		NULL, assign_temp_tablespaces, NULL
+ 	},
+ 
  	/* End-of-list marker */
  	{
  		{NULL, 0, 0, NULL, NULL}, NULL, NULL, NULL, NULL
Index: src/backend/utils/misc/postgresql.conf.sample
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/misc/postgresql.conf.sample,v
retrieving revision 1.204
retrieving revision 1.205
diff -c -r1.204 -r1.205
*** src/backend/utils/misc/postgresql.conf.sample	20 Jan 2007 21:42:03 -0000	1.204
--- src/backend/utils/misc/postgresql.conf.sample	25 Jan 2007 04:35:11 -0000	1.205
***************
*** 399,404 ****
--- 399,406 ----
  #search_path = '"$user",public'		# schema names
  #default_tablespace = ''		# a tablespace name, '' uses
  					# the default
+ #temp_tablespaces = ''			# a list of tablespace names,
+ 					# '' uses default_tablespace
  #check_function_bodies = on
  #default_transaction_isolation = 'read committed'
  #default_transaction_read_only = off
Index: src/include/commands/tablespace.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/commands/tablespace.h,v
retrieving revision 1.14
retrieving revision 1.15
diff -c -r1.14 -r1.15
*** src/include/commands/tablespace.h	5 Jan 2007 22:19:54 -0000	1.14
--- src/include/commands/tablespace.h	25 Jan 2007 04:35:11 -0000	1.15
***************
*** 41,46 ****
--- 41,47 ----
  extern void TablespaceCreateDbspace(Oid spcNode, Oid dbNode, bool isRedo);
  
  extern Oid	GetDefaultTablespace(void);
+ extern Oid	GetTempTablespace(void);
  
  extern Oid	get_tablespace_oid(const char *tablespacename);
  extern char *get_tablespace_name(Oid spc_oid);
Index: src/include/utils/guc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/guc.h,v
retrieving revision 1.78
retrieving revision 1.79
diff -c -r1.78 -r1.79
*** src/include/utils/guc.h	9 Jan 2007 21:31:17 -0000	1.78
--- src/include/utils/guc.h	25 Jan 2007 04:35:11 -0000	1.79
***************
*** 238,241 ****
--- 238,245 ----
  extern const char *assign_xlog_sync_method(const char *method,
  						bool doit, GucSource source);
  
+ /* in commands/tablespace.c */
+ extern const char *assign_temp_tablespaces(const char *newval,
+ 						  bool doit, GucSource source);
+ 
  #endif   /* GUC_H */
#6Jaime Casanova
systemguards@gmail.com
In reply to: Tom Lane (#4)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

On 3/5/07, Tom Lane <tgl@sss.pgh.pa.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?

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

#7Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jaime Casanova (#6)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

"Jaime Casanova" <systemguards@gmail.com> writes:

On 3/5/07, Tom Lane <tgl@sss.pgh.pa.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.

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

#8Jaime Casanova
systemguards@gmail.com
In reply to: Tom Lane (#7)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

On 3/17/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Jaime Casanova" <systemguards@gmail.com> writes:

On 3/5/07, Tom Lane <tgl@sss.pgh.pa.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.

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

#9Simon Riggs
simon@2ndquadrant.com
In reply to: Jaime Casanova (#8)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces toprovide a default location for

On Sun, 2007-03-18 at 14:05 -0500, Jaime Casanova wrote:

On 3/17/07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Jaime Casanova" <systemguards@gmail.com> writes:

On 3/5/07, Tom Lane <tgl@sss.pgh.pa.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.

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

#10Bruce Momjian
bruce@momjian.us
In reply to: Simon Riggs (#9)
Re: [COMMITTERS] pgsql: Add GUC temp_tablespaces toprovide a default location for

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 <tgl@sss.pgh.pa.us> wrote:

"Jaime Casanova" <systemguards@gmail.com> writes:

On 3/5/07, Tom Lane <tgl@sss.pgh.pa.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.

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

#11Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#5)
Re: Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

This has been saved for the 8.4 release:

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

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

Bruce Momjian wrote:

OK, patch reverted. Authors, would you please resubmit with fixes?
Thanks.

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

Tom Lane wrote:

momjian@postgresql.org (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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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

#12Jaime Casanova
systemguards@gmail.com
In reply to: Bruce Momjian (#11)
Re: Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

On 4/2/07, Bruce Momjian <bruce@momjian.us> wrote:

This has been saved for the 8.4 release:

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

mmm... sorry, i have been busy... how many time we have? i can send
something for friday...

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

#13Bruce Momjian
bruce@momjian.us
In reply to: Jaime Casanova (#12)
Re: Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

Jaime Casanova wrote:

On 4/2/07, Bruce Momjian <bruce@momjian.us> wrote:

This has been saved for the 8.4 release:

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

mmm... sorry, i have been busy... how many time we have? i can send
something for friday...

Yes. Friday is fine, or even Monday.

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

#14Bruce Momjian
bruce@momjian.us
In reply to: Jaime Casanova (#12)
Re: Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for

I think we will have to wait for 8.4 for this.

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

Jaime Casanova wrote:

On 4/2/07, Bruce Momjian <bruce@momjian.us> wrote:

This has been saved for the 8.4 release:

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

mmm... sorry, i have been busy... how many time we have? i can send
something for friday...

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

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