On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <
pg@bowt.ie> wrote:
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.
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.