Calling PrepareTempTablespaces in BufFileCreateTemp

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Calling PrepareTempTablespaces in BufFileCreateTemp

Melanie Plageman
Hi,
PrepareTempTablespaces is called by most callers of BufFileCreateTemp, so I was
wondering if there is a reason not to call it inside BufFileCreateTemp.

As a developer using BufFileCreateTemp to write code that will create spill
files, it was easy to forget the extra step of checking the temp_tablespaces
GUC to ensure I create the spill files there if it is set.

Thanks,
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Peter Geoghegan-4
On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
Peter Geoghegan <[hidden email]> writes:
> On Mon, Apr 22, 2019 at 3:12 PM Melanie Plageman
> <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
I wrote:
> Peter Geoghegan <[hidden email]> 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();
 
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Melanie Plageman

On Tue, Apr 23, 2019 at 1:06 PM Tom Lane <[hidden email]> wrote:
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?


I think an assertion is sufficiently clear for GetNextTempTableSpace based on
what it does and its current callers. The same is probably true for
GetTempTableSpaces.

--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
In reply to this post by Tom Lane-2
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 */
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Peter Geoghegan-4
On Wed, Apr 24, 2019 at 12:17 PM Tom Lane <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
Peter Geoghegan <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Ashwin Agrawal
In reply to this post by Peter Geoghegan-4


On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <[hidden email]> wrote:
On Wed, Apr 24, 2019 at 12:17 PM Tom Lane <[hidden email]> 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.

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. But with first approach fixes the issue
for current and future users, plus excersicing the same just with a single test
already tests it for future callers as well. So, that way first approach sounds
more promising if we are fetch between the two.

Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
Ashwin Agrawal <[hidden email]> writes:
> On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <[hidden email]> 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@...


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Melanie Plageman
In reply to this post by Ashwin Agrawal


On Thu, Apr 25, 2019 at 9:19 AM Ashwin Agrawal <[hidden email]> wrote:
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. But with first approach fixes the issue
for current and future users, plus excersicing the same just with a single test
already tests it for future callers as well. So, that way first approach sounds
more promising if we are fetch between the two.

Would an existing test cover the code after moving PrepareTempTablespaces into
OpenTemporaryFile?
 

--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Ashwin Agrawal
In reply to this post by Tom Lane-2

On Thu, Apr 25, 2019 at 9:45 AM Tom Lane <[hidden email]> wrote:
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 agree to that point, same logic should be used for all three calls irrespective of the approach we pick.

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?

Is there (easy) way to assert for that assumption? If yes, then can add the same and make it not rickety.

Though I agree any exceptions/violations coded generally bites in long run somewhere later.
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Michael Paquier-2
In reply to this post by Tom Lane-2
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@...
--
Michael

assert-that-temp-tablespaces-are-prepared-v2.patch (1K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Michael Paquier-2
In reply to this post by Ashwin Agrawal
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
In reply to this post by Michael Paquier-2
Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Michael Paquier-2
On Fri, Apr 26, 2019 at 11:05:11AM -0400, Tom Lane wrote:
> Michael Paquier <[hidden email]> 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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Ashwin Agrawal
In reply to this post by Michael Paquier-2
On Thu, Apr 25, 2019 at 11:53 PM Michael Paquier <[hidden email]> 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@...


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Peter Geoghegan-4
On Mon, Apr 29, 2019 at 12:31 PM Ashwin Agrawal <[hidden email]> 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@...
--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Melanie Plageman
In reply to this post by Tom Lane-2

On Fri, Apr 26, 2019 at 8:05 AM Tom Lane <[hidden email]> wrote:
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.

Running `make check` with [1] applied and one of the calls to
PrepareTempTablespaces commented out, I felt like I deserved more as a
developer than the assertion in this case.

Assertions are especially good to protect against regressions, but, in this
case, I'm just trying to use an API that is being provided.

Assertions don't give me a nice, easy-to-understand test failure. I see that
there was a crash halfway through make check and now I have to figure out why.

If that is the default way for developers to find out that they are missing
something when using the API, it would be nice if it gave me some sort of
understandable diff or error message.

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.

So, just to make sure I understand this case:

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

OpenTemporaryFile is concerned with temp tablespaces, so any reference to those
should be there.

However, PrepareTempTablespaces should not be called in OpenTemporaryFile
because it is in fd.c and no functions that make up part of the file descriptor
API should do catalog lookups.
Is this correct?

[1] https://www.postgresql.org/message-id/11777.1556133426%40sss.pgh.pa.us

--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Calling PrepareTempTablespaces in BufFileCreateTemp

Tom Lane-2
Melanie Plageman <[hidden email]> 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


12