Re: [HACKERS] [COMMITTERS] pgsql: Add GUC temp_tablespaces to provide a default location for - Mailing list pgsql-patches

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