On Fri, May 24, 2024 at 11:03 AM Michael Paquier <michael@paquier.xyz> wrote:
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.
If we are looking for avoiding a segfault and get a message which helps debugging, using get_attname and get_attnum might be better options. get_attname throws an error. get_attnum doesn't throw an error and returns InvalidAttnum which won't return any valid identity sequence, and thus return a NIL sequence list which is handled in that function already. Using these two functions will avoid the clutter as well as segfault. If that's acceptable, I will provide a patch.