Re: Proper object locking for GRANT/REVOKE

On Fri, Nov 15, 2024 at 11:19:50AM +0100, Peter Eisentraut wrote:
> On 11.11.24 08:53, Bertrand Drouvot wrote:

> Thanks. I have applied your patch and then also mine with the appropriate
> adjustments.

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

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?

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