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:
> 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".
>
> The tablespace changes correctly for me when creating temporary tables. 
> I've got some questions though:
>
>       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"?
>
>       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?
>
> Hope the diff and idents are ok. Please let me know if there's something
> wrong with them.
>
> Thanks!
Index: src/backend/commands/tablecmds.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.207
diff -c -r1.207 tablecmds.c
*** src/backend/commands/tablecmds.c	23 Dec 2006 00:43:09 -0000	1.207
--- src/backend/commands/tablecmds.c	28 Dec 2006 01:43:53 -0000
***************
*** 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: /projects/cvsroot/pgsql/src/backend/commands/tablespace.c,v
retrieving revision 1.39
diff -c -r1.39 tablespace.c
*** src/backend/commands/tablespace.c	4 Oct 2006 00:29:51 -0000	1.39
--- src/backend/commands/tablespace.c	28 Dec 2006 01:43:54 -0000
***************
*** 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,1068 ----
  	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;
+ 	/*
+ 	 * 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?
+ 		 */
+ 		foreach(l, namelist)
+ 		{
+ 			char	   *curname = (char *) lfirst(l);
+ 
+ 			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++;
+ 		}
+ 	}
+ 
+ 	pfree(rawname);
+ 	list_free(namelist);
+ 	next_temp_tablespace = 0;
+ 	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
***************
*** 1002,1008 ****
  	return result;
  }
  
- 
  /*
   * TABLESPACE resource manager's routines
   */
--- 1135,1140 ----
Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.131
diff -c -r1.131 fd.c
*** src/backend/storage/file/fd.c	6 Nov 2006 17:10:22 -0000	1.131
--- src/backend/storage/file/fd.c	28 Dec 2006 01:43:56 -0000
***************
*** 7,13 ****
   * Portions Copyright (c) 1994, Regents of the University of California
   *
   * IDENTIFICATION
!  *	  $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.131 2006/11/06 17:10:22 tgl Exp $
   *
   * NOTES:
   *
--- 7,13 ----
   * Portions Copyright (c) 1994, Regents of the University of California
   *
   * IDENTIFICATION
!  *	  $PostgreSQL: pgsql/src/backend/storage/file/fd.c,v 1.130 2006/10/04 00:29:57 momjian Exp $
   *
   * NOTES:
   *
***************
*** 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"
***************
*** 75,80 ****
--- 77,83 ----
   */
  #define FD_MINFREE				10
  
+ #define OIDCHARS        10                      /* max chars printed by %u */
  
  /*
   * A number of platforms allow individual processes to open many more files
***************
*** 879,924 ****
  {
  	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++);
  
! 	/*
! 	 * Open the file.  Note: we don't use O_EXCL, in case there is an orphaned
! 	 * temp file that can be reused.
! 	 */
! 	file = FileNameOpenFile(tempfilepath,
  							O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
  							0600);
! 	if (file <= 0)
! 	{
! 		char	   *dirpath;
  
  		/*
! 		 * We might need to create the pg_tempfiles subdirectory, if no one
! 		 * has yet done so.
! 		 *
! 		 * Don't check for error from mkdir; it could fail if someone else
! 		 * just did the same thing.  If it doesn't work then we'll bomb out on
! 		 * the second create attempt, instead.
  		 */
! 		dirpath = make_database_relative(PG_TEMP_FILES_DIR);
! 		mkdir(dirpath, S_IRWXU);
! 		pfree(dirpath);
  
  		file = FileNameOpenFile(tempfilepath,
  								O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
  								0600);
  		if (file <= 0)
! 			elog(ERROR, "could not create temporary file \"%s\": %m",
! 				 tempfilepath);
  	}
  
  	/* Mark it for deletion at close */
  	VfdCache[file].fdstate |= FD_TEMPORARY;
  
--- 882,967 ----
  {
  	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 = 10 + 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++);
  
+ 		/*
+ 		 * Open the file.  Note: we don't use O_EXCL, in case there is an orphaned
+ 		 * temp file that can be reused.
+ 		 */
  		file = FileNameOpenFile(tempfilepath,
  								O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
  								0600);
  		if (file <= 0)
! 		{
! 			char	   *dirpath;
! 
! 			/*
! 			 * We might need to create the pg_tempfiles subdirectory, if no one
! 			 * has yet done so.
! 			 *
! 			 * Don't check for error from mkdir; it could fail if someone else
! 			 * just did the same thing.  If it doesn't work then we'll bomb out on
! 			 * the second create attempt, instead.
! 			 */
! 			dirpath = make_database_relative(PG_TEMP_FILES_DIR);
! 			mkdir(dirpath, S_IRWXU);
! 			pfree(dirpath);
! 
! 			file = FileNameOpenFile(tempfilepath,
! 									O_RDWR | O_CREAT | O_TRUNC | PG_BINARY,
! 									0600);
! 			if (file <= 0)
! 				elog(ERROR, "could not create temporary file \"%s\": %m",
! 					 tempfilepath);
! 		}
  	}
  
+ 
  	/* Mark it for deletion at close */
  	VfdCache[file].fdstate |= FD_TEMPORARY;
  
***************
*** 1278,1283 ****
--- 1321,1340 ----
  		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: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.363
diff -c -r1.363 guc.c
*** src/backend/utils/misc/guc.c	23 Dec 2006 00:52:40 -0000	1.363
--- src/backend/utils/misc/guc.c	28 Dec 2006 01:44:01 -0000
***************
*** 97,102 ****
--- 97,103 ----
  extern int	CommitDelay;
  extern int	CommitSiblings;
  extern char *default_tablespace;
+ extern char *temp_tablespaces;
  extern bool fullPageWrites;
  
  #ifdef TRACE_SORT
***************
*** 2267,2272 ****
--- 2268,2283 ----
  		NULL, assign_canonical_path, NULL
  	},
  
+ 	{
+ 		{"temp_tablespaces", PGC_SUSET, 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/include/commands/tablespace.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/commands/tablespace.h,v
retrieving revision 1.13
diff -c -r1.13 tablespace.h
*** src/include/commands/tablespace.h	24 Mar 2006 04:32:13 -0000	1.13
--- src/include/commands/tablespace.h	28 Dec 2006 01:44:01 -0000
***************
*** 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: /projects/cvsroot/pgsql/src/include/utils/guc.h,v
retrieving revision 1.76
diff -c -r1.76 guc.h
*** src/include/utils/guc.h	19 Oct 2006 18:32:47 -0000	1.76
--- src/include/utils/guc.h	28 Dec 2006 01:44:02 -0000
***************
*** 237,240 ****
--- 237,244 ----
  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 */
---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Reply via email to