Parallell hashjoin sometimes ignores temp_tablespaces

> On 29 Jun 2020, at 17:02, Magnus Hagander <[hidden email]> wrote:

> I think the fix is the attached one (tested on version 11 which is what $customer is using).  To me it looks like this may have been a copy/paste error all the way back in 98e8b480532 which added default_tablespace back in 2004. (And is in itself entirely unrelated to parallel hashjoin, but that's where it got exposed at least in my case)

Running through the repro and patch on HEAD I confirm that the attached fixes
the issue. +1 for the patch and a backpatch of it.

It would be nice to have a test covering test_tablespaces, but it seems a tad
cumbersome to create a stable one.

cheers ./daniel

Magnus Hagander <[hidden email]> writes:
> Thanks. pushed!

Sorry for not having paid more attention earlier, but this patch is
quite broken.  If it weren't misguided it'd still be wrong, because
this isn't the only spot in PrepareTempTablespaces that inserts
InvalidOid into the output list.

But, in fact, it's intentional that we represent the DB's default
tablespace by InvalidOid in that list.  Some callers of
GetNextTempTableSpace need that to be the case, either for
permissions-checking reasons or because they're going to store the
result into a temp table's pg_class.reltablespace, where that
representation is *required*.

I see that this is perhaps underdocumented, since while
GetNextTempTableSpace's comment mentions the behavior, there's
no comment about it with the data structure proper.

It looks to me like the actual bug here is that whoever added
GetTempTablespaces() and made sharedfileset.c depend on it
did not get the memo about what to do with InvalidOid.
It's possible that we could safely make GetTempTablespaces()
do the substitution, but that would be making fd.c assume more
about the usage of GetTempTablespaces() than I think it ought to.
I feel like we oughta fix sharedfileset.c, instead.

                        regards, tom lane

Magnus Hagander <[hidden email]> writes:
> A quick look -- to do things right, we will need to know the database
> default tablespace in this case right? Which I guess isn't there because
> the shared fileset isn't tied to a database. But perhaps it's as easy as
> something like the attached, just overwriting the oid?

Yeah, we just have to pick an appropriate place for making the
substitution.  I have no objection to doing it in SharedFileSetInit, as
long as we're sure it will only be consulted for placing temp files and
not relations.

The lack of documentation seems to be my fault, so I'm willing to pick
this up unless somebody else wants it.

                        regards, tom lane

Magnus Hagander <[hidden email]> writes:
> On Fri, Jul 3, 2020 at 6:12 PM Tom Lane <[hidden email]> wrote:
>> The lack of documentation seems to be my fault, so I'm willing to pick
>> this up unless somebody else wants it.

> If the comments I included in that patch are enough, I can just commit
> those along with it. Otherwise, please do :)

Being once burned, I had something more like the attached in mind.

BTW, looking at this, I'm kind of wondering about the other code path
in SharedFileSetInit:

        if (fileset->ntablespaces == 0)
        {
                fileset->tablespaces[0] = DEFAULTTABLESPACE_OID;
                fileset->ntablespaces = 1;
        }

Shouldn't that be inserting MyDatabaseTableSpace?  I see no other places
anywhere that are forcing temp stuff into pg_default like this.

                        regards, tom lane


diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index f887ee9857..2c3b9050b2 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1183,6 +1183,7 @@ GetDefaultTablespace(char relpersistence, bool partitioned)
 
 typedef struct
 {
+ /* Array of OIDs to be passed to SetTempTablespaces() */
  int numSpcs;
  Oid tblSpcs[FLEXIBLE_ARRAY_MEMBER];
 } temp_tablespaces_extra;
@@ -1232,6 +1233,7 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source)
  /* Allow an empty string (signifying database default) */
  if (curname[0] == '\0')
  {
+ /* InvalidOid signifies database's default tablespace */
  tblSpcs[numSpcs++] = InvalidOid;
  continue;
  }
@@ -1258,6 +1260,7 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source)
  */
  if (curoid == MyDatabaseTableSpace)
  {
+ /* InvalidOid signifies database's default tablespace */
  tblSpcs[numSpcs++] = InvalidOid;
  continue;
  }
@@ -1368,6 +1371,7 @@ PrepareTempTablespaces(void)
  /* Allow an empty string (signifying database default) */
  if (curname[0] == '\0')
  {
+ /* InvalidOid signifies database's default tablespace */
  tblSpcs[numSpcs++] = InvalidOid;
  continue;
  }
@@ -1386,7 +1390,8 @@ PrepareTempTablespaces(void)
  */
  if (curoid == MyDatabaseTableSpace)
  {
- tblSpcs[numSpcs++] = curoid;
+ /* InvalidOid signifies database's default tablespace */
+ tblSpcs[numSpcs++] = InvalidOid;
  continue;
  }
 
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 7dc6dd2f15..5f6420efb2 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -264,8 +264,10 @@ static int numExternalFDs = 0;
 static long tempFileCounter = 0;
 
 /*
- * Array of OIDs of temp tablespaces.  When numTempTableSpaces is -1,
- * this has not been set in the current transaction.
+ * Array of OIDs of temp tablespaces.  (Some entries may be InvalidOid,
+ * indicating that the current database's default tablespace should be used.)
+ * When numTempTableSpaces is -1, this has not been set in the current
+ * transaction.
  */
 static Oid *tempTableSpaces = NULL;
 static int numTempTableSpaces = -1;
@@ -2779,6 +2781,9 @@ closeAllVfds(void)
  * unless this function is called again before then.  It is caller's
  * responsibility that the passed-in array has adequate lifespan (typically
  * it'd be allocated in TopTransactionContext).
+ *
+ * Some entries of the array may be InvalidOid, indicating that the current
+ * database's default tablespace should be used.
  */
 void
 SetTempTablespaces(Oid *tableSpaces, int numSpaces)
@@ -2818,7 +2823,10 @@ TempTablespacesAreSet(void)
  * GetTempTablespaces
  *
  * Populate an array with the OIDs of the tablespaces that should be used for
- * temporary files.  Return the number that were copied into the output array.
+ * temporary files.  (Some entries may be InvalidOid, indicating that the
+ * current database's default tablespace should be used.)  At most numSpaces
+ * entries will be filled.
+ * Returns the number of OIDs that were copied into the output array.
  */
 int
 GetTempTablespaces(Oid *tableSpaces, int numSpaces)
diff --git a/src/backend/storage/file/sharedfileset.c b/src/backend/storage/file/sharedfileset.c
index f7206c9175..0f65210476 100644
--- a/src/backend/storage/file/sharedfileset.c
+++ b/src/backend/storage/file/sharedfileset.c
@@ -66,6 +66,21 @@ SharedFileSetInit(SharedFileSet *fileset, dsm_segment *seg)
  fileset->tablespaces[0] = DEFAULTTABLESPACE_OID;
  fileset->ntablespaces = 1;
  }
+ else
+ {
+ int i;
+
+ /*
+ * An entry of InvalidOid means use the default tablespace for the
+ * current database.  Replace that now, to be sure that all users of
+ * the SharedFileSet agree on what to do.
+ */
+ for (i = 0; i < fileset->ntablespaces; i++)
+ {
+ if (fileset->tablespaces[i] == InvalidOid)
+ fileset->tablespaces[i] = MyDatabaseTableSpace;
+ }
+ }
 
  /* Register our cleanup callback. */
  on_dsm_detach(seg, SharedFileSetOnDetach, PointerGetDatum(fileset));

Magnus Hagander <[hidden email]> writes:
> On Fri, Jul 3, 2020 at 7:06 PM Tom Lane <[hidden email]> wrote:
>> Shouldn't that be inserting MyDatabaseTableSpace?  I see no other places
>> anywhere that are forcing temp stuff into pg_default like this.

> Yeah, looking at it again, I think it should. I can't see any reason why it
> should enforce pg_default.

OK, pushed with that additional correction.

                        regards, tom lane