Re: Calling PrepareTempTablespaces in BufFileCreateTemp

[Michael Paquier]

On Thu, Apr 25, 2019 at 12:45:03PM -0400, Tom Lane wrote:
> I still remain concerned that invoking catalog lookups from fd.c is a darn
> bad idea, even if we have a fallback for it to work (for some value of
> "work") in non-transactional states.  It's not really hard to envision
> that kind of thing leading to infinite recursion.  I think it's safe
> right now, because catalog fetches shouldn't lead to any temp-file
> access, but that's sort of a rickety assumption isn't it?

Introducing catalog lookups into fd.c which is not a layer designed
for that is a choice that I find strange, and I fear that it may bite
in the future.  I think that the choice proposed upthread to add
an assertion on TempTablespacesAreSet() when calling a function
working on temporary data is just but fine, and that we should just
make sure that the gist code calls PrepareTempTablespaces()
correctly.  So [1] is a proposal I find much more acceptable than the
other one.

I think that one piece is missing from the patch.  Wouldn't it be
better to add an assertion at the beginning of OpenTemporaryFile() to
make sure that PrepareTempTablespaces() has been called when interXact
is true?  We could just go with that:
Assert(!interXact || TempTablespacesAreSet());

And this gives me the attached.

[1]: https://postgr.es/m/11777.1556133...@sss.pgh.pa.us
--
Michael

diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 4f2363e21e..1d008ec43d 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -17,6 +17,7 @@
 #include "access/genam.h"
 #include "access/gist_private.h"
 #include "catalog/index.h"
+#include "commands/tablespace.h"
 #include "miscadmin.h"
 #include "storage/buffile.h"
 #include "storage/bufmgr.h"
@@ -58,6 +59,8 @@ gistInitBuildBuffers(int pagesPerBuffer, int levelStep, int maxLevel)
 	 * Create a temporary file to hold buffer pages that are swapped out of
 	 * memory.
 	 */
+	PrepareTempTablespaces();
+
 	gfbb->pfile = BufFileCreateTemp(false);
 	gfbb->nFileBlocks = 0;
 
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index fdac9850e0..d03ffce417 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -1465,6 +1465,12 @@ OpenTemporaryFile(bool interXact)
 {
 	File		file = 0;
 
+	/*
+	 * Unless interXact is true, callers of this function should have done
+	 * PrepareTempTablespaces().
+	 */
+	Assert(!interXact || TempTablespacesAreSet());
+
 	/*
 	 * Make sure the current resource owner has space for this File before we
 	 * open it, if we'll be registering it below.
@@ -1481,7 +1487,7 @@ OpenTemporaryFile(bool interXact)
 	 * force it into the database's default tablespace, so that it will not
 	 * pose a threat to possible tablespace drop attempts.
 	 */
-	if (numTempTableSpaces > 0 && !interXact)
+	if (!interXact)
 	{
 		Oid			tblspcOid = GetNextTempTableSpace();
 
@@ -2732,6 +2738,7 @@ GetTempTablespaces(Oid *tableSpaces, int numSpaces)
 Oid
 GetNextTempTableSpace(void)
 {
+	Assert(TempTablespacesAreSet());
 	if (numTempTableSpaces > 0)
 	{
 		/* Advance nextTempTableSpace counter with wraparound */

signature.asc
Description: PGP signature