On Wed, Apr 24, 2019 at 5:48 PM Peter Geoghegan <p...@bowt.ie> wrote:
> On Wed, Apr 24, 2019 at 12:17 PM Tom Lane <t...@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.
