Sort dependent objects before reporting them in DROP ROLE.

Commit 8aa9dd7 didn't quite finish the job in this area after all,
because DROP ROLE has a code path distinct from DROP OWNED BY, and
it was still reporting dependent objects in whatever order the index
scan returned them in.

Buildfarm experience shows that index ordering of equal-keyed objects is
significantly less stable than before in the wake of using heap TIDs as
tie-breakers.  So if we try to hide the unstable ordering by suppressing
DETAIL reports, we're just going to end up having to do that for every
DROP that reports multiple objects.  That's not great from a coverage
or problem-detection standpoint, and it's something we'll inevitably
forget in future patches, leading to more iterations of fixing-an-
unstable-result.  So let's just bite the bullet and sort here too.

Discussion: https://postgr.es/m/E1h6eep-0001Mw-Vd@gemulon.postgresql.org

Author: tglsfdc

src/backend/catalog/pg_shdepend.c

@@ -74,6 +74,13 @@ typedef enum
 	REMOTE_OBJECT
 } SharedDependencyObjectType;
 
+typedef struct
+{
+	ObjectAddress object;
+	char		deptype;
+	SharedDependencyObjectType objtype;
+} ShDependObjectInfo;
+
 static void getOidListDiff(Oid *list1, int *nlist1, Oid *list2, int *nlist2);
 static Oid	classIdGetDbId(Oid classId);
 static void shdepChangeDep(Relation sdepRel,
@@ -496,6 +503,56 @@ typedef struct
 	int			count;
 } remoteDep;
 
+/*
+ * qsort comparator for ShDependObjectInfo items
+ */
+static int
+shared_dependency_comparator(const void *a, const void *b)
+{
+	const ShDependObjectInfo *obja = (const ShDependObjectInfo *) a;
+	const ShDependObjectInfo *objb = (const ShDependObjectInfo *) b;
+
+	/*
+	 * Primary sort key is OID ascending.
+	 */
+	if (obja->object.objectId < objb->object.objectId)
+		return -1;
+	if (obja->object.objectId > objb->object.objectId)
+		return 1;
+
+	/*
+	 * Next sort on catalog ID, in case identical OIDs appear in different
+	 * catalogs.  Sort direction is pretty arbitrary here.
+	 */
+	if (obja->object.classId < objb->object.classId)
+		return -1;
+	if (obja->object.classId > objb->object.classId)
+		return 1;
+
+	/*
+	 * Sort on object subId.
+	 *
+	 * We sort the subId as an unsigned int so that 0 (the whole object) will
+	 * come first.
+	 */
+	if ((unsigned int) obja->object.objectSubId < (unsigned int) objb->object.objectSubId)
+		return -1;
+	if ((unsigned int) obja->object.objectSubId > (unsigned int) objb->object.objectSubId)
+		return 1;
+
+	/*
+	 * Last, sort on deptype, in case the same object has multiple dependency
+	 * types.  (Note that there's no need to consider objtype, as that's
+	 * determined by the catalog OID.)
+	 */
+	if (obja->deptype < objb->deptype)
+		return -1;
+	if (obja->deptype > objb->deptype)
+		return 1;
+
+	return 0;
+}
+
 /*
  * checkSharedDependencies
  *
@@ -531,16 +588,27 @@ checkSharedDependencies(Oid classId, Oid objectId,
 	List	   *remDeps = NIL;
 	ListCell   *cell;
 	ObjectAddress object;
+	ShDependObjectInfo *objects;
+	int			numobjects;
+	int			allocedobjects;
 	StringInfoData descs;
 	StringInfoData alldescs;
 
 	/*
 	 * We limit the number of dependencies reported to the client to
 	 * MAX_REPORTED_DEPS, since client software may not deal well with
 	 * enormous error strings.  The server log always gets a full report.
+	 *
+	 * For stability of regression test results, we sort local and shared
+	 * objects by OID before reporting them.  We don't worry about the order
+	 * in which other databases are reported, though.
 	 */
 #define MAX_REPORTED_DEPS 100
 
+	allocedobjects = 128;		/* arbitrary initial array size */
+	objects = (ShDependObjectInfo *)
+		palloc(allocedobjects * sizeof(ShDependObjectInfo));
+	numobjects = 0;
 	initStringInfo(&descs);
 	initStringInfo(&alldescs);
 
@@ -580,36 +648,26 @@ checkSharedDependencies(Oid classId, Oid objectId,
 
 		/*
 		 * If it's a dependency local to this database or it's a shared
-		 * object, describe it.
+		 * object, add it to the objects array.
 		 *
 		 * If it's a remote dependency, keep track of it so we can report the
 		 * number of them later.
 		 */
-		if (sdepForm->dbid == MyDatabaseId)
-		{
-			if (numReportedDeps < MAX_REPORTED_DEPS)
-			{
-				numReportedDeps++;
-				storeObjectDescription(&descs, LOCAL_OBJECT, &object,
-									   sdepForm->deptype, 0);
-			}
-			else
-				numNotReportedDeps++;
-			storeObjectDescription(&alldescs, LOCAL_OBJECT, &object,
-								   sdepForm->deptype, 0);
-		}
-		else if (sdepForm->dbid == InvalidOid)
+		if (sdepForm->dbid == MyDatabaseId ||
+			sdepForm->dbid == InvalidOid)
 		{
-			if (numReportedDeps < MAX_REPORTED_DEPS)
+			if (numobjects >= allocedobjects)
 			{
-				numReportedDeps++;
-				storeObjectDescription(&descs, SHARED_OBJECT, &object,
-									   sdepForm->deptype, 0);
+				allocedobjects *= 2;
+				objects = (ShDependObjectInfo *)
+					repalloc(objects,
+							 allocedobjects * sizeof(ShDependObjectInfo));
 			}
-			else
-				numNotReportedDeps++;
-			storeObjectDescription(&alldescs, SHARED_OBJECT, &object,
-								   sdepForm->deptype, 0);
+			objects[numobjects].object = object;
+			objects[numobjects].deptype = sdepForm->deptype;
+			objects[numobjects].objtype = (sdepForm->dbid == MyDatabaseId) ?
+				LOCAL_OBJECT : SHARED_OBJECT;
+			numobjects++;
 		}
 		else
 		{
@@ -647,6 +705,33 @@ checkSharedDependencies(Oid classId, Oid objectId,
 
 	table_close(sdepRel, AccessShareLock);
 
+	/*
+	 * Sort and report local and shared objects.
+	 */
+	if (numobjects > 1)
+		qsort((void *) objects, numobjects,
+			  sizeof(ShDependObjectInfo), shared_dependency_comparator);
+
+	for (int i = 0; i < numobjects; i++)
+	{
+		if (numReportedDeps < MAX_REPORTED_DEPS)
+		{
+			numReportedDeps++;
+			storeObjectDescription(&descs,
+								   objects[i].objtype,
+								   &objects[i].object,
+								   objects[i].deptype,
+								   0);
+		}
+		else
+			numNotReportedDeps++;
+		storeObjectDescription(&alldescs,
+							   objects[i].objtype,
+							   &objects[i].object,
+							   objects[i].deptype,
+							   0);
+	}
+
 	/*
 	 * Summarize dependencies in remote databases.
 	 */
@@ -670,6 +755,7 @@ checkSharedDependencies(Oid classId, Oid objectId,
 							   SHARED_DEPENDENCY_INVALID, dep->count);
 	}
 
+	pfree(objects);
 	list_free_deep(remDeps);
 
 	if (descs.len == 0)

src/test/regress/expected/dependency.out

@@ -128,8 +128,8 @@ FROM pg_type JOIN pg_class c ON typrelid = c.oid WHERE typname = 'deptest_t';
 -- doesn't work: grant still exists
 DROP USER regress_dep_user1;
 ERROR:  role "regress_dep_user1" cannot be dropped because some objects depend on it
-DETAIL:  privileges for table deptest1
-privileges for database regression
+DETAIL:  privileges for database regression
+privileges for table deptest1
 owner of default privileges on new relations belonging to role regress_dep_user1 in schema deptest
 DROP OWNED BY regress_dep_user1;
 DROP USER regress_dep_user1;