PrepareTempTablespaces directly before making temp file

PrepareTempTablespaces is called by almost all callers of
BufFileCreateTemp* and seems like it would be easier from a readability
perspective and as a developer when writing some code that is going to
make temp files, I would prefer not to have to remember to check and set
the value that the user set for temp tablespaces GUC. It seems like that
is an unnecessary chore that is easily forgotten.

For example, for HashAgg, we have our own spilling implementation and
forgot to call PrepareTempTablespaces, so spill files were not getting
created in the temp tablespace specified by the user.

Note that in the workfile_mgr_test, we use SetConfigOption to set the
temp_tablespaces GUC. It would also have worked to redeclare the
variable in the test file and then assign to it. However, SetConfigOption
is used when a user sets a GUC, so it seems more intuitive for the
reader of the test to use this.
Co-authored-by: NAshwin Agrawal <aagrawal@pivotal.io>

src/backend/executor/nodeHash.c

@@ -400,9 +400,6 @@ ExecHashTableCreate(HashState *hashState, HashJoinState *hjstate, List *hashOper
 		 */
 		hashtable->innerBatchFile = (BufFile **) 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();
 	}
 
 	/*
@@ -739,8 +736,6 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 		/* we had no file arrays before */
 		hashtable->innerBatchFile = (BufFile **) palloc0(nbatch * sizeof(BufFile *));
 		hashtable->outerBatchFile = (BufFile **) palloc0(nbatch * sizeof(BufFile *));
-		/* time to establish the temp tablespaces, too */
-		PrepareTempTablespaces();
 	}
 	else
 	{

src/backend/storage/file/buffile.c

@@ -63,6 +63,7 @@
 #include <zstd.h>
 #endif
 
+#include "commands/tablespace.h"
 #include "executor/instrument.h"
 #include "storage/fd.h"
 #include "storage/buffile.h"
@@ -200,7 +201,16 @@ BufFileCreateTempInSet(workfile_set *work_set, bool interXact)
 	char		filePrefix[MAXPGPATH];
 
 	snprintf(filePrefix, MAXPGPATH, "_%s_", work_set->prefix);
-
+	/*
+	 * In upstream, PrepareTempTablespaces() is called by callers of
+	 * BufFileCreateTemp*. Since we were burned once by forgetting to call it
+	 * for hyperhashagg spill files, we moved it into BufFileCreateTempInSet,
+	 * as we didn't see a reason not to.
+	 * We also posed the question upstream
+	 * https://www.postgresql.org/message-id/
+	 * CAAKRu_YwzjuGAmmaw4-8XO=OVFGR1QhY_Pq-t3wjb9ribBJb_Q@mail.gmail.com
+	 */
+	PrepareTempTablespaces();
 	pfile = OpenTemporaryFile(interXact, filePrefix);
 	Assert(pfile >= 0);
 

src/test/regress/input/workfile_mgr_test.source

@@ -16,4 +16,12 @@ $$
 $$
 LANGUAGE SQL;
 
+-- setup for workfile made in temp tablespace test
+\! mkdir -p '@testtablespace@/workfile_mgr';
+CREATE TABLESPACE ts1 LOCATION '@testtablespace@/workfile_mgr';
+
+SELECT gp_workfile_mgr_test('workfile_made_in_temp_tablespace');
+
+DROP TABLESPACE ts1;
+
 SELECT gp_workfile_mgr_test('fd_tests');

src/test/regress/output/workfile_mgr_test.source

@@ -13,6 +13,19 @@ $$
 	SELECT C.* FROM gp_workfile_mgr_test_on_segments($1) as C;
 $$
 LANGUAGE SQL;
+-- setup for workfile made in temp tablespace test
+\! mkdir -p '@testtablespace@/workfile_mgr';
+CREATE TABLESPACE ts1 LOCATION '@testtablespace@/workfile_mgr';
+SELECT gp_workfile_mgr_test('workfile_made_in_temp_tablespace');
+ gp_workfile_mgr_test 
+----------------------
+ t
+ t
+ t
+ t
+(4 rows)
+
+DROP TABLESPACE ts1;
 SELECT gp_workfile_mgr_test('fd_tests');
  gp_workfile_mgr_test 
 ----------------------

src/test/regress/workfile_mgr_test.c

@@ -24,10 +24,10 @@
 #include "port/atomics.h"
 #include "storage/buffile.h"
 #include "utils/builtins.h"
+#include "utils/guc.h"
 #include "utils/logtape.h"
 #include "utils/memutils.h"
 
-
 #define TEST_NAME_LENGTH 50
 #define TEST_HT_NUM_ELEMENTS 8192
 
@@ -49,6 +49,7 @@ static bool execworkfile_create_one_MB_file(void);
 static bool workfile_fill_sharedcache(void);
 static bool workfile_create_and_set_cleanup(void);
 static bool workfile_create_and_individual_cleanup(void);
+static bool workfile_made_in_temp_tablespace(void);
 
 static bool atomic_test(void);
 
@@ -81,6 +82,7 @@ static test_def test_defns[] = {
 		{"workfile_fill_sharedcache", workfile_fill_sharedcache},
 		{"workfile_create_and_set_cleanup", workfile_create_and_set_cleanup},
 		{"workfile_create_and_individual_cleanup", workfile_create_and_individual_cleanup},
+		{"workfile_made_in_temp_tablespace", workfile_made_in_temp_tablespace},
 		{NULL, NULL}, /* This has to be the last element of the array */
 };
 
@@ -810,6 +812,49 @@ workfile_create_and_set_cleanup(void)
 	return unit_test_summary();
 }
 
+static bool
+workfile_made_in_temp_tablespace(void)
+{
+	bool success = true;
+
+	unit_test_reset();
+
+	/*
+	 * Set temp_tablespaces at a session level to simulate what a user does
+	 */
+	SetConfigOption("temp_tablespaces", "ts1", PGC_INTERNAL, PGC_S_SESSION);
+
+	workfile_set *work_set = workfile_mgr_create_set("workfile_test", NULL);
+
+	/*
+	 * BufFileCreateTempInSet calls PrepareTempTablespaces
+	 * which parses the temp_tablespaces value and BufFileCreateTempInSet
+	 * uses that value as the location for workfile created
+	 */
+	BufFile *bufFile = BufFileCreateTempInSet(work_set, false);
+
+	if (bufFile == NULL)
+		success = false;
+
+	const char *bufFilePath = BufFileGetFilename(bufFile);
+
+	char *expectedPathPrefix = "pg_tblspc/";
+
+	/*
+	 * Expect workfile to be created in the temp tablespace specified above,
+	 * which will have the prefix, "pg_tblspc". By default,
+	 * workfiles are created in data directory having prefix, "base"
+	 */
+	if(0 != strncmp(bufFilePath, expectedPathPrefix, strlen(expectedPathPrefix)))
+		success = false;
+
+	BufFileClose(bufFile);
+	workfile_mgr_close_set(work_set);
+
+	unit_test_result(success);
+
+	return unit_test_summary();
+}
 
 /*
  * Unit test that creates very many workfiles, and then closes them one by one