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