Re: Proper object locking for GRANT/REVOKE - Mailing list pgsql-hackers
From | Bertrand Drouvot |
---|---|
Subject | Re: Proper object locking for GRANT/REVOKE |
Date | |
Msg-id | ZyOTsX6QUgIz65ip@ip-10-97-1-34.eu-west-3.compute.internal Whole thread Raw |
List | pgsql-hackers |
Hi, On Mon, Oct 28, 2024 at 04:20:42PM +0100, Peter Eisentraut wrote: > This patch started out as a refactoring, thinking that objectNamesToOids() > in aclchk.c should really mostly be a loop around get_object_address(). > This is mostly true, with a few special cases because the node > representations are a bit different in some cases, and OBJECT_PARAMETER_ACL, > which is obviously very different. This saves a bunch of duplicative code, > which is nice. > > Additionally, get_object_address() handles locking, which > objectNamesToOids() somewhat famously does not do, and there is a code > comment about it. With this refactoring, we get the locking pretty much for > free. Thanks for the patch, this refactoring makes sense to me. A few random comments: 1 === + default: I like the idea of using default as the first "case" as a way to emphasize that this is the most "common" behavior. 2 === Nit + address = get_object_address(objtype, lfirst(cell), &relation, lockmode, false); + Assert(relation == NULL); Worth to explain why we do expect relation to be NULL here? (the comment on top of get_object_address() says it all, but maybe a few words here could be worth it). 3 === - - /* Used GRANT DOMAIN on a non-domain? */ - if (istmt->objtype == OBJECT_DOMAIN && - pg_type_tuple->typtype != TYPTYPE_DOMAIN) - ereport(ERROR, - (errcode(ERRCODE_WRONG_OBJECT_TYPE), - errmsg("\"%s\" is not a domain", - NameStr(pg_type_tuple->typname)))); Yeah, get_object_address()->get_object_address_type() does take care of it. 4 === > > Interestingly, this changes the output of the intra-grant-inplace isolation > test, which is recent work. It might be good to get some review from those > who worked on that, to make sure that the new behavior is still correct > and/or to check whether those test cases are still applicable. -s4: WARNING: got: cache lookup failed for relation REDACTED +s4: WARNING: got: relation "intra_grant_inplace" does not exist I did not work on this but out of curiosity I looked at it, and IIUC this change comes from: - relOid = RangeVarGetRelid(relvar, NoLock, false); + relOid = RangeVarGetRelid(relvar, lockmode, false); So without the lock, the error is coming from ExecGrant_Relation() that way: [local] DO(+0x22529b) [0x5c56afdb629b] [local] DO(+0x2220b0) [0x5c56afdb30b0] [local] DO(ExecuteGrantStmt+0x696) [0x5c56afdb3047] [local] DO(+0x6dc8cb) [0x5c56b026d8cb] [local] DO(standard_ProcessUtility+0xd38) [0x5c56b026b9da] [local] DO(ProcessUtility+0x13a) [0x5c56b026ac9b] [local] DO(+0x460073) [0x5c56afff1073] With the lock, the error comes from RangeVarGetRelidExtended() that way: [local] DO(RangeVarGetRelidExtended+0x4e6) [0x5a1c3ac49d36] [local] DO(+0x2223fe) [0x5a1c3ac2a3fe] [local] DO(ExecuteGrantStmt+0x111) [0x5a1c3ac29ac2] [local] DO(+0x6dc1d2) [0x5a1c3b0e41d2] [local] DO(standard_ProcessUtility+0xd38) [0x5a1c3b0e22e1] [local] DO(ProcessUtility+0x13a) [0x5a1c3b0e15a2] That's due to (in RangeVarGetRelidExtended()): " * But if lockmode = NoLock, then we assume that either the caller is OK * with the answer changing under them, or that they already hold some * appropriate lock, and therefore return the first answer we get without * checking for invalidation messages. " So, in the RangeVarGetRelid(relvar, NoLock, false) case (without the patch) then if we are able to reach "relId = RelnameGetRelid(relation->relname);" before the drop gets committed then we get the "cache lookup failed" error. But if we are slow enough so that we don't reach "relId = RelnameGetRelid(relation->relname);" before the drop get committed then we would also get the "relation "intra_grant_inplace" does not exist" error (tested manually, attaching gdb on s4 and breakpoint before the RelnameGetRelid(relation->relname) call in RangeVarGetRelidExtended()). The fact that we use lockmode != NoLock in the patch produces a lock followed by a "retry" in RangeVarGetRelidExtended() and so we get the "relation "intra_grant_inplace" does not exist" error. I think that the new behavior is still correct and in fact is more "appropriate" ( I mean that's the kind of error I expect to see from a user point of view). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
pgsql-hackers by date: