Re: Proper object locking for GRANT/REVOKE

On 25.11.24 02:24, Noah Misch wrote:
> commit d31bbfb wrote:
>> --- a/src/backend/catalog/aclchk.c
>> +++ b/src/backend/catalog/aclchk.c
>> @@ -659,147 +659,77 @@ ExecGrantStmt_oids(InternalGrant *istmt)
>> * objectNamesToOids
>> *
>> * Turn a list of object names of a given type into an Oid list.
>> - *
>> - * XXX: This function doesn't take any sort of locks on the objects whose
>> - * names it looks up. 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.
>
> To prevent "latch onto an old version" and remove the last sentence of the
> comment, we'd need two more things:
>
> - Use a self-exclusive lock here, not AccessShareLock. With two sessions
> doing GRANT under AccessShareLock, one will "latch onto an old version".
>
> - Use a self-exclusive lock before *every* CatalogTupleUpdate() of a row that
> GRANT/REVOKE can affect. For example, today's locking in ALTER FUNCTION is
> the xmax stamped on the old tuple. If GRANT switched to
> ShareUpdateExclusiveLock, concurrent ALTER FUNCTION could still cause GRANT
> to "latch onto an old version".

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."

> I wouldn't do those, however. It would make GRANT ALL TABLES IN SCHEMA
> terminate every autovacuum running in the schema and consume a shared lock
> table entry per table in the schema. I think the user-visible benefit of
> commit d31bbfb plus this additional work is just changing "ERROR: tuple
> concurrently updated" to blocking. That's not nothing, but I don't see it
> outweighing autovacuum termination and lock table consumption spikes. What
> other benefits and drawbacks should we weigh?

I think what are describing is a reasonable tradeoff. The user
experience is tolerable: "tuple concurrently updated" is a mildly useful
error message, and it's probably the table owner executing both commands.

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).

>> --- 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?