Backport #8466 to release-12.1: Fix role propagation error due to missing grantor dependencies

src/backend/distributed/metadata/dependency.c

@@ -186,10 +186,13 @@ static void ApplyAddCitusDependedObjectsToDependencyList(
 static List * GetViewRuleReferenceDependencyList(Oid relationId);
 static List * ExpandCitusSupportedTypes(ObjectAddressCollector *collector,
 										ObjectAddress target);
+static List * ExpandCitusSupportedTypesForNodeActivation(ObjectAddressCollector *
+														 collector,
+														 ObjectAddress target);
 static List * ExpandForPgVanilla(ObjectAddressCollector *collector,
 								 ObjectAddress target);
 static List * GetDependentRoleIdsFDW(Oid FDWOid);
-static List * ExpandRolesToGroups(Oid roleid);
+static List * ExpandRolesToGroups(Oid roleid, bool includeGrantors);
 static ViewDependencyNode * BuildViewDependencyGraph(Oid relationId, HTAB *nodeMap);
 static bool IsObjectAddressOwnedByExtension(const ObjectAddress *target,
 											ObjectAddress *extensionAddress);
@@ -343,7 +346,7 @@ OrderObjectAddressListInDependencyOrder(List *objectAddressList)
 		}
 
 		RecurseObjectDependencies(*objectAddress,
-								  &ExpandCitusSupportedTypes,
+								  &ExpandCitusSupportedTypesForNodeActivation,
 								  &FollowAllSupportedDependencies,
 								  &ApplyAddToDependencyList,
 								  &collector);
@@ -1544,9 +1547,15 @@ ExpandCitusSupportedTypes(ObjectAddressCollector *collector, ObjectAddress targe
 		{
 			/*
 			 * Roles are members of other roles. These relations are not recorded directly
-			 * but can be deduced from pg_auth_members
+			 * but can be deduced from pg_auth_members.
+			 *
+			 * Note: we intentionally do NOT include grantors here. Grantors are
+			 * only relevant for ordering role creation during node activation
+			 * (see ExpandCitusSupportedTypesForNodeActivation). Including them
+			 * in the generic dependency graph would produce false-positive
+			 * circular-dependency errors for legitimate mutual GRANTs.
 			 */
-			return ExpandRolesToGroups(target.objectId);
+			return ExpandRolesToGroups(target.objectId, false);
 		}
 
 		case ExtensionRelationId:
@@ -1738,6 +1747,32 @@ ExpandCitusSupportedTypes(ObjectAddressCollector *collector, ObjectAddress targe
 }
 
 
+/*
+ * ExpandCitusSupportedTypesForNodeActivation is a variant of
+ * ExpandCitusSupportedTypes used only when ordering distributed objects for
+ * propagation to a newly-activated node. For roles, it additionally treats
+ * grantors of membership tuples as dependencies so that a role used as a
+ * grantor is created on the new node before the grantee role. This is what
+ * fixes issue #8425.
+ *
+ * This expansion must NOT be used by the generic dependency-collection paths
+ * (creation, cycle-detection, DDL propagation) because mutually-granted roles
+ * would appear to form a cycle and be rejected by
+ * DeferErrorIfCircularDependencyExists.
+ */
+static List *
+ExpandCitusSupportedTypesForNodeActivation(ObjectAddressCollector *collector,
+										   ObjectAddress target)
+{
+	if (target.classId == AuthIdRelationId)
+	{
+		return ExpandRolesToGroups(target.objectId, true);
+	}
+
+	return ExpandCitusSupportedTypes(collector, target);
+}
+
+
 /*
  * ExpandForPgVanilla only expands only comosite types because other types
  * will find their dependencies in pg_depend. The method should only be called by
@@ -1801,10 +1836,19 @@ GetDependentRoleIdsFDW(Oid FDWOid)
 
 /*
  * ExpandRolesToGroups returns a list of object addresses pointing to roles that roleid
- * depends on.
+ * depends on. This always includes:
+ *   1. Roles that roleid is a member of (membership->roleid)
+ *
+ * When includeGrantors is true, it additionally includes:
+ *   2. Roles that are used as grantors for roleid's memberships (membership->grantor)
+ *
+ * The grantor dependency is only used for ordering role propagation during node
+ * activation (see ExpandCitusSupportedTypesForNodeActivation). It must NOT be used
+ * in the generic dependency graph because legitimate mutual GRANTs between roles
+ * would otherwise be reported as circular dependencies.
  */
 static List *
-ExpandRolesToGroups(Oid roleid)
+ExpandRolesToGroups(Oid roleid, bool includeGrantors)
 {
 	Relation pgAuthMembers = table_open(AuthMemRelationId, AccessShareLock);
 	HeapTuple tuple = NULL;
@@ -1820,15 +1864,47 @@ ExpandRolesToGroups(Oid roleid)
 													true, NULL, scanKeyCount, scanKey);
 
 	List *roles = NIL;
+
+	/*
+	 * Track all role OIDs we have already emitted as dependencies so that
+	 * parent roles and grantors are de-duplicated through a single set.
+	 * A role can appear multiple times in pg_auth_members for the same
+	 * member (different grantors), and the same OID may show up as both a
+	 * parent role and a grantor; one DependencyDefinition per OID is enough.
+	 *
+	 * Note: For roles with many memberships this O(n) membership check could
+	 * be replaced with a hash set, but in practice the number of memberships
+	 * per role is small.
+	 */
+	List *seenRoleIds = NIL;
 	while ((tuple = systable_getnext(scanDescriptor)) != NULL)
 	{
 		Form_pg_auth_members membership = (Form_pg_auth_members) GETSTRUCT(tuple);
 
-		DependencyDefinition *definition = palloc0(sizeof(DependencyDefinition));
-		definition->mode = DependencyObjectAddress;
-		ObjectAddressSet(definition->data.address, AuthIdRelationId, membership->roleid);
+		Oid candidates[2] = { membership->roleid, membership->grantor };
+		int numCandidates = includeGrantors ? 2 : 1;
+		for (int i = 0; i < numCandidates; i++)
+		{
+			Oid candidateOid = candidates[i];
 
-		roles = lappend(roles, definition);
+			/*
+			 * Skip self-references: a role cannot depend on itself (the
+			 * parent-role case cannot hit this because pg_auth_members does
+			 * not allow roleid == member, but the grantor case can).
+			 */
+			if (candidateOid == roleid ||
+				!OidIsValid(candidateOid) ||
+				list_member_oid(seenRoleIds, candidateOid))
+			{
+				continue;
+			}
+
+			DependencyDefinition *definition = palloc0(sizeof(DependencyDefinition));
+			definition->mode = DependencyObjectAddress;
+			ObjectAddressSet(definition->data.address, AuthIdRelationId, candidateOid);
+			roles = lappend(roles, definition);
+			seenRoleIds = lappend_oid(seenRoleIds, candidateOid);
+		}
 	}
 
 	systable_endscan(scanDescriptor);

src/test/regress/expected/create_role_propagation.out

@@ -696,3 +696,61 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1;
 (0 rows)
 
 \c - - - :master_port
+-- test interdependent roles with grantor dependencies
+-- This test recreates the issue where role1 is used as a grantor for role2's grants,
+-- but role1 hasn't been granted admin option on the parent role yet.
+SELECT master_remove_node('localhost', :worker_2_port);
+ master_remove_node
+---------------------------------------------------------------------
+
+(1 row)
+
+CREATE ROLE read_only_role;
+CREATE ROLE interdep_role1;
+CREATE ROLE interdep_role2;
+-- Grant read_only_role to interdep_role1 WITH ADMIN OPTION
+-- This allows interdep_role1 to grant read_only_role to other roles
+GRANT read_only_role TO interdep_role1 WITH ADMIN OPTION;
+-- Grant read_only_role to interdep_role2, using interdep_role1 as the grantor
+-- Also make interdep_role1 a member of interdep_role2 with admin option
+GRANT read_only_role TO interdep_role2 GRANTED BY interdep_role1;
+GRANT interdep_role1 TO interdep_role2 WITH ADMIN OPTION;
+-- Verify the grant relationships on coordinator
+SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option
+FROM pg_auth_members
+WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2')
+   OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2')
+ORDER BY role, member;
+      role      |     member     |    grantor     | admin_option
+---------------------------------------------------------------------
+ interdep_role1 | interdep_role2 | postgres       | t
+ read_only_role | interdep_role1 | postgres       | t
+ read_only_role | interdep_role2 | interdep_role1 | f
+(3 rows)
+
+-- Add worker_2 back - this should succeed with our fix
+-- Before the fix, this would fail because interdep_role2 would be propagated before
+-- interdep_role1's admin option on read_only_role was set up
+SELECT 1 FROM master_add_node('localhost', :worker_2_port);
+ ?column?
+---------------------------------------------------------------------
+        1
+(1 row)
+
+-- Verify the grants were properly propagated to worker_2
+\c - - - :worker_2_port
+SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option
+FROM pg_auth_members
+WHERE roleid::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2')
+   OR member::regrole::text IN ('read_only_role', 'interdep_role1', 'interdep_role2')
+ORDER BY role, member;
+      role      |     member     |    grantor     | admin_option
+---------------------------------------------------------------------
+ interdep_role1 | interdep_role2 | postgres       | t
+ read_only_role | interdep_role1 | postgres       | t
+ read_only_role | interdep_role2 | postgres       | f
+(3 rows)
+
+\c - - - :master_port
+-- Clean up interdependent roles
+DROP ROLE interdep_role2, interdep_role1, read_only_role;