Re: Proper object locking for GRANT/REVOKE

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.

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.

>>>> --- a/src/test/isolation/expected/intra-grant-inplace.out
>>>> +++ b/src/test/isolation/expected/intra-grant-inplace.out
>>>> @@ -248,6 +248,6 @@ relhasindex
>>>>    -----------
>>>>    (0 rows)
>>>> -s4: WARNING:  got: cache lookup failed for relation REDACTED
>>>> +s4: WARNING:  got: relation "intra_grant_inplace" does not exist
>>>
>>> The affected permutation existed to cover the first LockRelease() in
>>> SearchSysCacheLocked1().  Since this commit, that line no longer has coverage.
>>
>> Do you have an idea how such a test case could be constructed now?
> 
> A rough idea.  The test worked because REVOKE used only LOCKTAG_TUPLE, which
> didn't mind the LOCKTAG_RELATION from DROP TABLE.
> 
> One route might be to find another SearchSysCacheLocked1() caller that takes
> no locks beyond the LOCKTAG_TUPLE taken right in SearchSysCacheLocked1().  I'd
> add a temporary elog to report if that's happening.
> check_lock_if_inplace_updateable_rel() is an example of reporting the absence
> of a lock.  If check-world w/ that elog finds some operation reaching that
> circumstance, this test could replace REVOKE with that operation.
> 
> Another route would be to remove the catalog row without locking the
> underlying object, e.g. by replacing DROP TABLE with DELETE FROM pg_class.
> That's more artificial.

I have attached here a patch that shows that the last idea does work.

I don't know what the best solution here is.  It seems weird to leave 
some higher-level code faulty in order to be able to test lower-level 
code that attempts to cope with the faults of the higher-level code.  I 
know that backstops have value, though.