Re: Calling PrepareTempTablespaces in BufFileCreateTemp

On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:

> On Wed, Apr 24, 2019 at 12:17 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>

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.