Fix role propagation error due to missing grantor dependencies (#8466)

DESCRIPTION: Add grantor role dependencies to fix node addition with interdependent roles

Modified ExpandRolesToGroups in dependency.c to track grantor roles as
dependencies alongside role memberships during node activation. This
ensures grantors are propagated before roles that reference them in
GRANT ... GRANTED BY statements.

Added regression test covering the interdependent role scenario.

Fixes #8425

Author: ihalatci

src/backend/distributed/metadata/dependency.c

@@ -187,10 +187,13 @@ static void ApplyAddCitusDependedObjectsToDependencyList(ObjectAddressCollector
 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);
@@ -344,7 +347,7 @@ OrderObjectAddressListInDependencyOrder(List *objectAddressList)
 		}
 
 		RecurseObjectDependencies(*objectAddress,
-								  &ExpandCitusSupportedTypes,
+								  &ExpandCitusSupportedTypesForNodeActivation,
 								  &FollowAllSupportedDependencies,
 								  &ApplyAddToDependencyList,
 								  &collector);
@@ -1545,9 +1548,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:
@@ -1737,6 +1746,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
@@ -1800,10 +1835,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;
@@ -1819,15 +1863,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

@@ -337,10 +337,11 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;
 
 \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 LIKE '%dist\_%' ORDER BY 1, 2;
-      role       |   member    | grantor  | admin_option
+      role       |     member      | grantor  | admin_option
 ---------------------------------------------------------------------
- non_dist_role_4 | dist_role_4 | postgres | f
-(1 row)
+ dist_role_1     | non_dist_role_1 | postgres | t
+ non_dist_role_4 | dist_role_4     | postgres | f
+(2 rows)
 
 SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;
      rolname
@@ -349,8 +350,9 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%dist\_%' ORDER BY 1;
  dist_role_2
  dist_role_3
  dist_role_4
+ non_dist_role_1
  non_dist_role_4
-(5 rows)
+(6 rows)
 
 \c - - - :master_port
 DROP ROLE dist_role_3, non_dist_role_3, dist_role_4, non_dist_role_4;
@@ -750,4 +752,62 @@ 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 | interdep_role1 | f
+(3 rows)
+
+\c - - - :master_port
+-- Clean up interdependent roles
+DROP ROLE interdep_role2, interdep_role1, read_only_role;
 DROP ROLE nondist_cascade_1, nondist_cascade_2, nondist_cascade_3, dist_cascade;

src/test/regress/sql/create_role_propagation.sql

@@ -320,4 +320,48 @@ SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1;
 SELECT rolname FROM pg_authid WHERE rolname LIKE '%existing%' ORDER BY 1;
 \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);
+
+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;
+
+-- 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);
+
+-- 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;
+
+\c - - - :master_port
+
+-- Clean up interdependent roles
+DROP ROLE interdep_role2, interdep_role1, read_only_role;
+
 DROP ROLE nondist_cascade_1, nondist_cascade_2, nondist_cascade_3, dist_cascade;