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
signature.asc
Description: PGP signature
