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?
