Обсуждение: Calling PrepareTempTablespaces in BufFileCreateTemp

On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I was
> wondering if there is a reason not to call it inside BufFileCreateTemp.

The best answer I can think of is that a BufFileCreateTemp() caller
might not want to do catalog access. Perhaps the contortions within
assign_temp_tablespaces() are something that callers ought to opt in
to explicitly.

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.

-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>> PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I was
>> wondering if there is a reason not to call it inside BufFileCreateTemp.

> The best answer I can think of is that a BufFileCreateTemp() caller
> might not want to do catalog access. Perhaps the contortions within
> assign_temp_tablespaces() are something that callers ought to opt in
> to explicitly.

It's kind of hard to see a reason to call it outside a transaction,
and even if we did, there are provisions for it not to go boom.

> 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.

            regards, tom lane

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();

I wrote:
> 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.

After a bit more thought it seemed like another answer would be to
make all three of those functions assert that the caller did the
right thing, as per attached.  This addresses the layering-violation
complaint, but might be more of a pain in the rear for developers.

Not really sure which way I like better.

            regards, tom lane

diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 4f2363e..1d008ec 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 fdac985..a4463ba 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -1459,6 +1459,9 @@ PathNameDeleteTemporaryDir(const char *dirname)
  * outlive the transaction that created them, so this should be false -- but
  * if you need "somewhat" temporary storage, this might be useful. In either
  * case, the file is removed when the File is explicitly closed.
+ *
+ * Unless interXact is true, some caller function should have done
+ * PrepareTempTablespaces().
  */
 File
 OpenTemporaryFile(bool interXact)
@@ -1481,7 +1484,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 +2735,7 @@ GetTempTablespaces(Oid *tableSpaces, int numSpaces)
 Oid
 GetNextTempTableSpace(void)
 {
+    Assert(TempTablespacesAreSet());
     if (numTempTableSpaces > 0)
     {
         /* Advance nextTempTableSpace counter with wraparound */

On Wed, Apr 24, 2019 at 12:17 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> After a bit more thought it seemed like another answer would be to
> make all three of those functions assert that the caller did the
> right thing, as per attached.  This addresses the layering-violation
> complaint, but might be more of a pain in the rear for developers.

In what sense is it not already a layering violation to call
PrepareTempTablespaces() as often as we do? PrepareTempTablespaces()
parses and validates the GUC variable and passes it to fd.c, but to me
that seems almost the same as calling the fd.c function
SetTempTablespaces() directly. PrepareTempTablespaces() allocates
memory that it won't free itself within TopTransactionContext. I'm not
seeing why the context that the PrepareTempTablespaces() catalog
access occurs in actually matters.

Like you, I find it hard to prefer one of the approaches over the
other, though I don't really know how to assess this layering
business. I'm glad that either approach will prevent oversights,
though.
-- 
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> ... I'm not
> seeing why the context that the PrepareTempTablespaces() catalog
> access occurs in actually matters.

The point there is that a catalog access might leak some amount of
memory.  Probably not enough to be a big deal, but ...

            regards, tom lane

Ashwin Agrawal <aagrawal@pivotal.io> writes:
> On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> Like you, I find it hard to prefer one of the approaches over the
>> other, though I don't really know how to assess this layering
>> business. I'm glad that either approach will prevent oversights,
>> though.

> Just to provide my opinion, since we are at intersection and can go
> either way on this. Second approach (just adding assert) only helps
> if the code path for ALL future callers gets excersied and test exist for
> the same, to expose potential breakage.

In view of the fact that the existing regression tests fail to expose the
need for gistInitBuildBuffers to worry about this [1], that's a rather
strong point.  It's hard to believe that somebody writing new code would
fail to notice such an assertion, but it's more plausible that later
rearrangements could break things and not notice due to lack of coverage.

However, by that argument we should change all 3 of these functions to
set up the data.  If we're eating the layering violation to the extent
of letting OpenTemporaryFile call into commands/tablespace, then there's
little reason for the other 2 not to do likewise.

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?

            regards, tom lane

[1] https://postgr.es/m/24954.1556130678@sss.pgh.pa.us

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.1556133426@sss.pgh.pa.us
--
Michael

On Thu, Apr 25, 2019 at 10:40:01AM -0700, Ashwin Agrawal wrote:
> Is there (easy) way to assert for that assumption? If yes, then can add the
> same and make it not rickety.

IsTransactionState() would be enough?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> 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());

The version that I posted left it to GetNextTempTableSpace to assert
that.  That seemed cleaner to me than an Assert that has to depend
on interXact.

            regards, tom lane

On Fri, Apr 26, 2019 at 11:05:11AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> The version that I posted left it to GetNextTempTableSpace to assert
> that.  That seemed cleaner to me than an Assert that has to depend
> on interXact.

Okay, no objections for that approach as well.  Are you planning to do
something about this thread for v12? It seems like the direction to take
is pretty clear, at least from my perspective.
--
Michael

On Thu, Apr 25, 2019 at 11:53 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Well the one thing I wish to point out explicitly is just taking fd.c
changes from [1], and running make check hits no assertions and
doesn't flag issue exist for gistbuildbuffers.c. Means its missing
coverage and in future same can happen as well.

[1]: https://postgr.es/m/11777.1556133426@sss.pgh.pa.us

On Mon, Apr 29, 2019 at 12:31 PM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> Well the one thing I wish to point out explicitly is just taking fd.c
> changes from [1], and running make check hits no assertions and
> doesn't flag issue exist for gistbuildbuffers.c. Means its missing
> coverage and in future same can happen as well.

I believe that the test coverage of GiST index builds is something
that is being actively worked on right now. It's a recognized problem
[1].

[1] https://postgr.es/m/24954.1556130678@sss.pgh.pa.us
-- 
Peter Geoghegan

Melanie Plageman <melanieplageman@gmail.com> writes:
> I also think that if there is a step that a caller should always take before
> calling a function, then there needs to be a very compelling reason not to
> move that step into the function itself.

Fair complaint.

> PrepareTempTablespaces should not be called in BufFileCreateTemp because
> it is not concerned with temp tablespaces.

Actually, my reason for thinking that was mostly "that won't fix the
problem, because what about other callers of OpenTemporaryFile?"

However, looking around, there aren't any others --- buffile.c is it.

So maybe a reasonable compromise is to add the Assert(s) in fd.c as
per previous patch, but *also* add PrepareTempTablespaces in
BufFileCreateTemp, so that at least users of buffile.c are insulated
from the issue.  buffile.c is still kind of low-level, but it's not
part of core infrastructure in the same way as fd.c, so probably I could
hold my nose for this solution from the system-structural standpoint.

            regards, tom lane

I wrote:
> So maybe a reasonable compromise is to add the Assert(s) in fd.c as
> per previous patch, but *also* add PrepareTempTablespaces in
> BufFileCreateTemp, so that at least users of buffile.c are insulated
> from the issue.  buffile.c is still kind of low-level, but it's not
> part of core infrastructure in the same way as fd.c, so probably I could
> hold my nose for this solution from the system-structural standpoint.

Actually, after digging around in the related code some more, I'm having
second thoughts about those Asserts.  PrepareTempTablespaces is pretty
clear about what it thinks the contract is:

    /*
     * Can't do catalog access unless within a transaction.  This is just a
     * safety check in case this function is called by low-level code that
     * could conceivably execute outside a transaction.  Note that in such a
     * scenario, fd.c will fall back to using the current database's default
     * tablespace, which should always be OK.
     */
    if (!IsTransactionState())
        return;

If we just add the discussed assertions and leave this bit alone,
the net effect would be that any tempfile usage outside a transaction
would suffer an assertion failure, *even if* it had called
PrepareTempTablespaces.  There doesn't seem to be any such usage in
the core code, but do we really want to forbid the case?  It seems
like fd.c shouldn't be imposing such a restriction, if it never has
before.

So now I'm feeling more favorable about the idea of adding a
PrepareTempTablespaces call to BufFileCreateTemp, and just stopping
with that.  If we want to do more, I feel like it requires a
significant amount of rethinking about what the expectations are for
fd.c, and some rejiggering of PrepareTempTablespaces's API too.
I'm not sufficiently excited about this issue to do that.

            regards, tom lane

On Tue, Apr 30, 2019 at 3:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So now I'm feeling more favorable about the idea of adding a
> PrepareTempTablespaces call to BufFileCreateTemp, and just stopping
> with that.  If we want to do more, I feel like it requires a
> significant amount of rethinking about what the expectations are for
> fd.c, and some rejiggering of PrepareTempTablespaces's API too.
> I'm not sufficiently excited about this issue to do that.

+1. Let's close this one out.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Apr 30, 2019 at 3:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So now I'm feeling more favorable about the idea of adding a
>> PrepareTempTablespaces call to BufFileCreateTemp, and just stopping
>> with that.  If we want to do more, I feel like it requires a
>> significant amount of rethinking about what the expectations are for
>> fd.c, and some rejiggering of PrepareTempTablespaces's API too.
>> I'm not sufficiently excited about this issue to do that.

> +1. Let's close this one out.

Will do so tomorrow.  Should we back-patch this?

            regards, tom lane

On Fri, May 17, 2019 at 6:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Will do so tomorrow.  Should we back-patch this?

I wouldn't, because I see no reason to. Somebody else might.

-- 
Peter Geoghegan

On Fri, May 17, 2019 at 06:37:22PM -0700, Peter Geoghegan wrote:
> On Fri, May 17, 2019 at 6:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Will do so tomorrow.  Should we back-patch this?
>
> I wouldn't, because I see no reason to. Somebody else might.

FWIW, I see no reason either for a back-patch.
--
Michael