From af6550d34466b3093edda54a0cc5a6f220d321b7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 24 Mar 2019 18:17:41 -0400 Subject: [PATCH] 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 --- src/backend/catalog/pg_shdepend.c | 132 +++++++++++++++++++---- src/test/regress/expected/dependency.out | 4 +- 2 files changed, 111 insertions(+), 25 deletions(-) diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c index 63a7f4838f..4a9b4efb05 100644 --- 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 8d31110b87..2f04b712a4 100644 --- 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; -- 2.40.0