Sort dependent objects before reporting them in DROP ROLE.

Commit 8aa9dd74b 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

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index 63a7f4838f818e278e1e16c90ece3da9613f3ff0..4a9b4efb0501dee4aaef10a9aa03261e6c862c50 100644 (file)
--- a/src/backend/catalog/pg_shdepend.c
+++ b/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,6 +588,9 @@ checkSharedDependencies(Oid classId, Oid objectId,
        List       *remDeps = NIL;
        ListCell   *cell;
        ObjectAddress object;
+       ShDependObjectInfo *objects;
+       int                     numobjects;
+       int                     allocedobjects;
        StringInfoData descs;
        StringInfoData alldescs;
 
@@ -538,9 +598,17 @@ checkSharedDependencies(Oid classId, Oid objectId,
         * 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)

diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out
index 8d31110b87418a38fe5e7dd31af8be65b04bbe00..2f04b712a4f57052bcd33ecf4da417a4b7388852 100644 (file)
--- a/src/test/regress/expected/dependency.out
+++ b/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;