Re: Proper object locking for GRANT/REVOKE

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.