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.
