Re: Proper object locking for GRANT/REVOKE

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