Re: Calling PrepareTempTablespaces in BufFileCreateTemp

I wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
>> That doesn't seem like a particularly good or complete answer, though.
>> Perhaps it should simply be called within BufFileCreateTemp(). The
>> BufFile/fd.c layering is confusing in a number of ways IMV.

> I don't actually see why BufFileCreateTemp should do it; if
> we're to add a call, seems like OpenTemporaryFile is the place,
> as that's what is really concerned with the temp tablespace(s).
> I'm in favor of doing this, I think, as it sure looks to me like
> gistInitBuildBuffers() is calling BufFileCreateTemp without any
> closely preceding PrepareTempTablespaces.  So we already have an
> instance of Melanie's bug in core.  It'd be difficult to notice
> because of the silent-fallback-to-default-tablespace behavior.

Here's a draft patch for that.

It's slightly ugly that this adds a dependency on commands/tablespace
to fd.c, which is a pretty low-level module.  I think wanting to avoid
that layering violation might've been the reason for doing things the
way they are.  However, this gets rid of tablespace dependencies in
some other files that are only marginally higher-level, like
tuplesort.c, so I'm not sure how strong that objection is.

There are three functions in fd.c that have a dependency on the
temp tablespace info having been set up:
    OpenTemporaryFile
    GetTempTablespaces
    GetNextTempTableSpace
This patch makes the first of those automatically set up the info
if it's not done yet.  The second one has always had an assertion
that the caller did it already, and now the third one does too.
An about equally plausible change would be to make all three
call PrepareTempTablespaces, but there are so few callers of the
second and third that I'm not sure that'd be better.  Thoughts?

            regards, tom lane

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 64eec91..db436e0 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -29,7 +29,6 @@
 #include "access/htup_details.h"
 #include "access/parallel.h"
 #include "catalog/pg_statistic.h"
-#include "commands/tablespace.h"
 #include "executor/execdebug.h"
 #include "executor/hashjoin.h"
 #include "executor/nodeHash.h"
@@ -570,9 +569,7 @@ ExecHashTableCreate(HashState *state, List *hashOperators, List *hashCollations,
             palloc0(nbatch * sizeof(BufFile *));
         hashtable->outerBatchFile = (BufFile **)
             palloc0(nbatch * sizeof(BufFile *));
-        /* The files will not be opened until needed... */
-        /* ... but make sure we have temp tablespaces established for them */
-        PrepareTempTablespaces();
+        /* The files will not be opened until needed */
     }

     MemoryContextSwitchTo(oldcxt);
@@ -917,8 +914,6 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
             palloc0(nbatch * sizeof(BufFile *));
         hashtable->outerBatchFile = (BufFile **)
             palloc0(nbatch * sizeof(BufFile *));
-        /* time to establish the temp tablespaces, too */
-        PrepareTempTablespaces();
     }
     else
     {
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fdac985..da89f2b 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -83,6 +83,7 @@
 #include "access/xact.h"
 #include "access/xlog.h"
 #include "catalog/pg_tablespace.h"
+#include "commands/tablespace.h"
 #include "common/file_perm.h"
 #include "pgstat.h"
 #include "portability/mem.h"
@@ -1466,6 +1467,14 @@ OpenTemporaryFile(bool interXact)
     File        file = 0;

     /*
+     * If we want to use a temp tablespace, make sure that info has been set
+     * up in the current transaction.  (This is harmless if we're not in a
+     * transaction, and it's also cheap if the info is already set up.)
+     */
+    if (!interXact)
+        PrepareTempTablespaces();
+
+    /*
      * Make sure the current resource owner has space for this File before we
      * open it, if we'll be registering it below.
      */
@@ -2732,6 +2741,7 @@ GetTempTablespaces(Oid *tableSpaces, int numSpaces)
 Oid
 GetNextTempTableSpace(void)
 {
+    Assert(TempTablespacesAreSet());
     if (numTempTableSpaces > 0)
     {
         /* Advance nextTempTableSpace counter with wraparound */
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 3eebd9e..bd9f215 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -101,7 +101,6 @@
 #include "access/nbtree.h"
 #include "catalog/index.h"
 #include "catalog/pg_am.h"
-#include "commands/tablespace.h"
 #include "executor/executor.h"
 #include "miscadmin.h"
 #include "pg_trace.h"
@@ -2464,13 +2463,6 @@ inittapestate(Tuplesortstate *state, int maxTapes)
     if (tapeSpace + GetMemoryChunkSpace(state->memtuples) < state->allowedMem)
         USEMEM(state, tapeSpace);

-    /*
-     * Make sure that the temp file(s) underlying the tape set are created in
-     * suitable temp tablespaces.  For parallel sorts, this should have been
-     * called already, but it doesn't matter if it is called a second time.
-     */
-    PrepareTempTablespaces();
-
     state->mergeactive = (bool *) palloc0(maxTapes * sizeof(bool));
     state->tp_fib = (int *) palloc0(maxTapes * sizeof(int));
     state->tp_runs = (int *) palloc0(maxTapes * sizeof(int));
diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index 0f38e7c..5c9d977 100644
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -811,6 +811,9 @@ tuplestore_puttuple_common(Tuplestorestate *state, void *tuple)
             /*
              * Nope; time to switch to tape-based operation.  Make sure that
              * the temp file(s) are created in suitable temp tablespaces.
+             * (Note: we could leave it to fd.c to do PrepareTempTablespaces,
+             * but it seems better to do it here, so that any catalog access
+             * is done with the caller's resources not the tuplestore's.)
              */
             PrepareTempTablespaces();