Subject: Proper object locking for GRANT/REVOKE

From: Peter Eisentraut <[email-masked]>
Date: Mon, 28 Oct 2024 16:20:42 +0100

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.

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.

Also, it would be worth thinking about what the correct lock mode should 
be here.
From fb910c6af0fb0691448680ec4a4cc1eb1857cd5b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Mon, 28 Oct 2024 16:07:01 +0100
Subject: [PATCH] Proper object locking for GRANT/REVOKE

Refactor objectNamesToOids() to use get_object_address() internally if
possible.  Not only does this save a lot of code, it also allows us to
use the object locking provided by get_object_address() for
GRANT/REVOKE.  There was previously a code comment that complained
about the lack of locking in objectNamesToOids(), which is now fixed.

The check in ExecGrant_Type_check() is obsolete because
get_object_address_type() already does the same check.
---
 src/backend/catalog/aclchk.c                  | 162 +++++-------------
 .../expected/intra-grant-inplace.out          |   2 +-
 2 files changed, 41 insertions(+), 123 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 95eb0b12277..2f91aa865a3 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -659,147 +659,76 @@ 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.
  */
 static List *
 objectNamesToOids(ObjectType objtype, List *objnames, bool is_grant)
 {
 	List	   *objects = NIL;
 	ListCell   *cell;
+	const LOCKMODE lockmode = AccessShareLock;
 
 	Assert(objnames != NIL);
 
 	switch (objtype)
 	{
+		default:
+
+			/*
+			 * For most object types, we use get_object_address() directly.
+			 */
+			foreach(cell, objnames)
+			{
+				ObjectAddress address;
+				Relation	relation;
+
+				address = get_object_address(objtype, lfirst(cell), &relation, lockmode, false);
+				Assert(relation == NULL);
+				objects = lappend_oid(objects, address.objectId);
+			}
+			break;
 		case OBJECT_TABLE:
 		case OBJECT_SEQUENCE:
+
+			/*
+			 * Here, we don't use get_object_address().  It requires that the
+			 * specified object type match the actual type of the object, but
+			 * in GRANT/REVOKE, all table-like things are addressed as TABLE.
+			 */
 			foreach(cell, objnames)
 			{
 				RangeVar   *relvar = (RangeVar *) lfirst(cell);
 				Oid			relOid;
 
-				relOid = RangeVarGetRelid(relvar, NoLock, false);
+				relOid = RangeVarGetRelid(relvar, lockmode, false);
 				objects = lappend_oid(objects, relOid);
 			}
 			break;
-		case OBJECT_DATABASE:
-			foreach(cell, objnames)
-			{
-				char	   *dbname = strVal(lfirst(cell));
-				Oid			dbid;
-
-				dbid = get_database_oid(dbname, false);
-				objects = lappend_oid(objects, dbid);
-			}
-			break;
 		case OBJECT_DOMAIN:
 		case OBJECT_TYPE:
+
+			/*
+			 * The parse representation of types and domains in privilege
+			 * targets is different from that expected by get_object_address()
+			 * (for parse conflict reasons), so we have to do a bit of
+			 * conversion here.
+			 */
 			foreach(cell, objnames)
 			{
 				List	   *typname = (List *) lfirst(cell);
-				Oid			oid;
+				TypeName   *tn = makeTypeNameFromNameList(typname);
+				ObjectAddress address;
+				Relation	relation;
 
-				oid = typenameTypeId(NULL, makeTypeNameFromNameList(typname));
-				objects = lappend_oid(objects, oid);
-			}
-			break;
-		case OBJECT_FUNCTION:
-			foreach(cell, objnames)
-			{
-				ObjectWithArgs *func = (ObjectWithArgs *) lfirst(cell);
-				Oid			funcid;
-
-				funcid = LookupFuncWithArgs(OBJECT_FUNCTION, func, false);
-				objects = lappend_oid(objects, funcid);
-			}
-			break;
-		case OBJECT_LANGUAGE:
-			foreach(cell, objnames)
-			{
-				char	   *langname = strVal(lfirst(cell));
-				Oid			oid;
-
-				oid = get_language_oid(langname, false);
-				objects = lappend_oid(objects, oid);
-			}
-			break;
-		case OBJECT_LARGEOBJECT:
-			foreach(cell, objnames)
-			{
-				Oid			lobjOid = oidparse(lfirst(cell));
-
-				if (!LargeObjectExists(lobjOid))
-					ereport(ERROR,
-							(errcode(ERRCODE_UNDEFINED_OBJECT),
-							 errmsg("large object %u does not exist",
-									lobjOid)));
-
-				objects = lappend_oid(objects, lobjOid);
-			}
-			break;
-		case OBJECT_SCHEMA:
-			foreach(cell, objnames)
-			{
-				char	   *nspname = strVal(lfirst(cell));
-				Oid			oid;
-
-				oid = get_namespace_oid(nspname, false);
-				objects = lappend_oid(objects, oid);
-			}
-			break;
-		case OBJECT_PROCEDURE:
-			foreach(cell, objnames)
-			{
-				ObjectWithArgs *func = (ObjectWithArgs *) lfirst(cell);
-				Oid			procid;
-
-				procid = LookupFuncWithArgs(OBJECT_PROCEDURE, func, false);
-				objects = lappend_oid(objects, procid);
-			}
-			break;
-		case OBJECT_ROUTINE:
-			foreach(cell, objnames)
-			{
-				ObjectWithArgs *func = (ObjectWithArgs *) lfirst(cell);
-				Oid			routid;
-
-				routid = LookupFuncWithArgs(OBJECT_ROUTINE, func, false);
-				objects = lappend_oid(objects, routid);
-			}
-			break;
-		case OBJECT_TABLESPACE:
-			foreach(cell, objnames)
-			{
-				char	   *spcname = strVal(lfirst(cell));
-				Oid			spcoid;
-
-				spcoid = get_tablespace_oid(spcname, false);
-				objects = lappend_oid(objects, spcoid);
-			}
-			break;
-		case OBJECT_FDW:
-			foreach(cell, objnames)
-			{
-				char	   *fdwname = strVal(lfirst(cell));
-				Oid			fdwid = get_foreign_data_wrapper_oid(fdwname, false);
-
-				objects = lappend_oid(objects, fdwid);
-			}
-			break;
-		case OBJECT_FOREIGN_SERVER:
-			foreach(cell, objnames)
-			{
-				char	   *srvname = strVal(lfirst(cell));
-				Oid			srvid = get_foreign_server_oid(srvname, false);
-
-				objects = lappend_oid(objects, srvid);
+				address = get_object_address(objtype, (Node *) tn, &relation, lockmode, false);
+				Assert(relation == NULL);
+				objects = lappend_oid(objects, address.objectId);
 			}
 			break;
 		case OBJECT_PARAMETER_ACL:
+
+			/*
+			 * Parameters are handled completely differently.
+			 */
 			foreach(cell, objnames)
 			{
 				/*
@@ -830,9 +759,6 @@ objectNamesToOids(ObjectType objtype, List *objnames, bool is_grant)
 					objects = lappend_oid(objects, parameterId);
 			}
 			break;
-		default:
-			elog(ERROR, "unrecognized GrantStmt.objtype: %d",
-				 (int) objtype);
 	}
 
 	return objects;
@@ -2458,14 +2384,6 @@ ExecGrant_Type_check(InternalGrant *istmt, HeapTuple tuple)
 				(errcode(ERRCODE_INVALID_GRANT_OPERATION),
 				 errmsg("cannot set privileges of multirange types"),
 				 errhint("Set the privileges of the range type instead.")));
-
-	/* 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))));
 }
 
 static void
diff --git a/src/test/isolation/expected/intra-grant-inplace.out b/src/test/isolation/expected/intra-grant-inplace.out
index 4e9695a0214..1aa9da622da 100644
--- 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
 step revoke4: <... completed>
 step r3: ROLLBACK;
-- 
2.47.0

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 31 Oct 2024 14:26:57 +0000

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

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

-

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.

I did not work on this but out of curiosity I looked at it, and IIUC this change
comes from:

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

---

From: Peter Eisentraut <[email-masked]>
Date: Sat, 9 Nov 2024 13:43:13 +0100

> of get_object_address() says it all, but maybe a few words here could be worth

There are several other callers with this pattern.

Maybe it would be better to push the assertion into 
get_object_address(), something like

     Assert(!relation || relp)

near the end.  Meaning, if you pass NULL for the relp argument, then you 
don't expect a relation.  This is kind of what will happen now anyway, 
except with a segfault instead of an assertion.

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 11 Nov 2024 07:53:05 +0000

Hi,

> There are several other callers with this pattern.

Right. And most of those places declare a Relation prior calling get_object_address()
_only_ for a following assertion.

> Maybe it would be better to push the assertion into get_object_address(),
> something like
> 
> near the end.

That looks like a good idea to me, that would make the code cleaner and easier
to understand.

> Meaning, if you pass NULL for the relp argument, then you

Yeah, I like it.

So, something like the attached (provided as .txt file to no mess up the CF bot
entry related to this thread) could be applied before?

Regards,

>From e0766ce3403eba54f28df140dd787b049d5eaf86 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Mon, 11 Nov 2024 07:07:02 +0000
Subject: [PATCH v1] Add an assertion in get_object_address()

Some places declared a Relation before calling get_object_address() only to
assert that the relation is NULL after the call.

The new assertion allows passing NULL as the relation argument at those places
making the code cleaner and easier to understand.
---
 src/backend/catalog/objectaddress.c |  8 +++++---
 src/backend/commands/alter.c        | 15 ++++-----------
 2 files changed, 9 insertions(+), 14 deletions(-)
  48.8% src/backend/catalog/
  51.1% src/backend/commands/

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 85a7b7e641..e089582285 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -896,8 +896,8 @@ static void getRelationIdentity(StringInfo buffer, Oid relid, List **object,
  *
  * If the object is a relation or a child object of a relation (e.g. an
  * attribute or constraint), the relation is also opened and *relp receives
- * the open relcache entry pointer; otherwise, *relp is set to NULL.  This
- * is a bit grotty but it makes life simpler, since the caller will
+ * the open relcache entry pointer (if *relp is not passed as a NULL argument).
+ * This is a bit grotty but it makes life simpler, since the caller will
  * typically need the relcache entry too.  Caller must close the relcache
  * entry when done with it.  The relation is locked with the specified lockmode
  * if the target object is the relation itself or an attribute, but for other
@@ -1204,8 +1204,10 @@ get_object_address(ObjectType objtype, Node *object,
 		old_address = address;
 	}
 
+	Assert(!relation || relp);
 	/* Return the object address and the relation. */
-	*relp = relation;
+	if (relp)
+		*relp = relation;
 	return address;
 }
 
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 4f99ebb447..a45f3bb6b8 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -421,13 +421,11 @@ ExecRenameStmt(RenameStmt *stmt)
 			{
 				ObjectAddress address;
 				Relation	catalog;
-				Relation	relation;
 
 				address = get_object_address(stmt->renameType,
 											 stmt->object,
-											 &relation,
+											 NULL,
 											 AccessExclusiveLock, false);
-				Assert(relation == NULL);
 
 				catalog = table_open(address.classId, RowExclusiveLock);
 				AlterObjectRename_internal(catalog,
@@ -482,8 +480,7 @@ ExecAlterObjectDependsStmt(AlterObjectDependsStmt *stmt, ObjectAddress *refAddre
 		table_close(rel, NoLock);
 
 	refAddr = get_object_address(OBJECT_EXTENSION, (Node *) stmt->extname,
-								 &rel, AccessExclusiveLock, false);
-	Assert(rel == NULL);
+								 NULL, AccessExclusiveLock, false);
 	if (refAddress)
 		*refAddress = refAddr;
 
@@ -563,16 +560,14 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt,
 		case OBJECT_TSTEMPLATE:
 			{
 				Relation	catalog;
-				Relation	relation;
 				Oid			classId;
 				Oid			nspOid;
 
 				address = get_object_address(stmt->objectType,
 											 stmt->object,
-											 &relation,
+											 NULL,
 											 AccessExclusiveLock,
 											 false);
-				Assert(relation == NULL);
 				classId = address.classId;
 				catalog = table_open(classId, RowExclusiveLock);
 				nspOid = LookupCreationNamespace(stmt->newschema);
@@ -876,15 +871,13 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
 		case OBJECT_TSDICTIONARY:
 		case OBJECT_TSCONFIGURATION:
 			{
-				Relation	relation;
 				ObjectAddress address;
 
 				address = get_object_address(stmt->objectType,
 											 stmt->object,
-											 &relation,
+											 NULL,
 											 AccessExclusiveLock,
 											 false);
-				Assert(relation == NULL);
 
 				AlterObjectOwner_internal(address.classId, address.objectId,
 										  newowner);
-- 
2.34.1

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 15 Nov 2024 11:19:50 +0100

> entry related to this thread) could be applied before?

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