On Fri, May 24, 2024 at 11:03 AM Michael Paquier <mich...@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. -- Best Wishes, Ashutosh Bapat
