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@sss.pgh.pa.us
--
Michael