On Thu, May 23, 2024 at 08:54:12AM -0300, Ranier Vilela wrote:
> All calls to functions like SearchSysCacheAttName, in the whole codebase,
> checks if returns are valid.
> It must be for a very strong reason, such a style.
Usually good practice, as I've outlined once upthread, because we do
expect the attributes to exist in this case. Or if you want, an error
is better than a crash if a concurrent path causes this area to lead
to inconsistent lookups, which is something I've seen in the past
while hacking on my own stuff, or just fix other things causing
syscache lookup inconsistencies. You'd be surprised to hear that
dropped attributes being mishandled is not that uncommon, especially
in out-of-core code, as one example. FWIW, I don't see much a point
in using ereport(), the two checks ought to be elog()s pointing to an
internal error as these two errors should never happen. Still, it is
a good idea to check that they never happen: aka an internal
error state is better than a crash if a problem arises.
> So, v3, implements it this way.
I don't understand the point behind the open/close of attrelation,
TBH. That's not needed.
Except fot these two points, this is just moving the calls to make
sure that we have valid tuples from the syscache, which is a better
practice. 509199587df7 is recent enough that this should be fixed now
rather than later.
--
Michael