On Wed, Jun 11, 2025 at 05:22:53PM +0200, Peter Eisentraut wrote: > On 09.12.24 02:25, Noah Misch wrote: > > > Ok, we should probably put that comment back in slightly altered form, > > > like > > > > > > "XXX This function intentionally takes only an AccessShareLock ... > > > $REASON. > > > In the face of concurrent DDL, we might easily latch > > > onto an old version of an object, causing the GRANT or REVOKE statement > > > to fail." > > > > Yep. > > There is an open item for PG18 for this. So here is a patch that adds a > comment back, mostly from your descriptions in this thread. > > > > The change to AccessShareLock at least prevents confusing "cache lookup > > > failed" messages, and might alleviate some security concerns about > > > swapping > > > in a completely different object concurrently (even if we currently think > > > this is not an actual problem). > > > > Perhaps. To me, the v17 behavior smells mildly superior to the v18 > > behavior. > > Hmm. I think there has been a general effort to get rid of internal errors > like "cache lookup failed ..." and replace them with proper user-facing > errors. This change seems in line with that.
I have seen some commits along those lines, e.g. d8a0993. They weren't adding lock acquisitions, though. If we've deliberately incurred lock acquisitions just to get rid of "cache lookup failed ...", I don't remember those commits. > An alternative, if we wanted to go back to the old behavior (other than > reverting altogether, since I think the refactoring is still valuable), > would be to allow get_object_address() to work with lockmode == NoLock. That > would require a bit of work, but nothing magical. That seems a bit better to me than your comment-only proposal, but either could be okay.
