rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Sort the dependent objects before recursing in findDependentObjects(). 

Historically, the notices output by DROP CASCADE tended to come out
in uncertain order, and in some cases you might get different claims
about which object depends on which other one.  This is because we
just traversed the dependency tree in the order in which pg_depend
entries are seen, and nbtree has never promised anything about the
order of equal-keyed index entries.  We've put up with that for years,
hacking regression tests when necessary to prevent them from emitting
unstable output.  However, it's a problem for pending work that will
change nbtree's behavior for equal keys, as that causes unexpected
changes in the regression test results.

Hence, adjust findDependentObjects to sort the results of each
indexscan before processing them.  The sort is on descending OID of
the dependent objects, hence more or less reverse creation order.
While this rule could still result in bogus regression test failures
if an OID wraparound occurred mid-test, that seems unlikely to happen
in any plausible development or packaging-test scenario.

This is enough to ensure output stability for ordinary DROP CASCADE
commands, but not for DROP OWNED BY, because that has a different
code path with the same problem.  We might later choose to sort in
the DROP OWNED BY code as well, but this patch doesn't do so.

I've also not done anything about reverting the existing hacks to
suppress unstable DROP CASCADE output in specific regression tests.
It might be worth undoing those, but it seems like a distinct question.

The first indexscan loop in findDependentObjects is not touched,
meaning there is a hazard of unstable error reports from that too.
However, said hazard is not the fault of that code: it was designed
on the assumption that there could be at most one "owning" object
to complain about, and that assumption does not seem unreasonable.
The recent patch that added the possibility of multiple
DEPENDENCY_INTERNAL_AUTO links broke that assumption, but we should
fix that situation not band-aid around it.  That's a matter for
another patch, though.

Discussion: https://postgr.es/m/12244.1547854440@sss.pgh.pa.us
This commit is contained in:
Tom Lane 2019-01-21 13:48:07 -05:00
parent fcea1e1090
commit f1ad067fc3
6 changed files with 98 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

100
src/backend/catalog/dependency.c
View File

@ -124,6 +124,13 @@ typedef struct ObjectAddressStack
struct ObjectAddressStack *next; /* next outer stack level */
} ObjectAddressStack;
/* temporary storage in findDependentObjects */
typedef struct
{
ObjectAddress obj; /* object to be deleted --- MUST BE FIRST */
int subflags; /* flags to pass down when recursing to obj */
} ObjectAddressAndFlags;
/* for find_expr_references_walker */
typedef struct
{
@ -472,6 +479,9 @@ findDependentObjects(const ObjectAddress *object,
SysScanDesc scan;
HeapTuple tup;
ObjectAddress otherObject;
ObjectAddressAndFlags *dependentObjects;
int numDependentObjects;
int maxDependentObjects;
ObjectAddressStack mystack;
ObjectAddressExtra extra;
@ -704,12 +714,17 @@ findDependentObjects(const ObjectAddress *object,
systable_endscan(scan);
/*
* Now recurse to any dependent objects. We must visit them first since
* they have to be deleted before the current object.
* Next, identify all objects that directly depend on the current object.
* To ensure predictable deletion order, we collect them up in
* dependentObjects and sort the list before actually recursing. (The
* deletion order would be valid in any case, but doing this ensures
* consistent output from DROP CASCADE commands, which is helpful for
* regression testing.)
*/
mystack.object = object; /* set up a new stack level */
mystack.flags = objflags;
mystack.next = stack;
maxDependentObjects = 128; /* arbitrary initial allocation */
dependentObjects = (ObjectAddressAndFlags *)
palloc(maxDependentObjects * sizeof(ObjectAddressAndFlags));
numDependentObjects = 0;
ScanKeyInit(&key[0],
Anum_pg_depend_refclassid,
@ -762,7 +777,10 @@ findDependentObjects(const ObjectAddress *object,
continue;
}
/* Recurse, passing objflags indicating the dependency type */
/*
* We do need to delete it, so identify objflags to be passed down,
* which depend on the dependency type.
*/
switch (foundDep->deptype)
{
case DEPENDENCY_NORMAL:
@ -798,8 +816,47 @@ findDependentObjects(const ObjectAddress *object,
break;
}
findDependentObjects(&otherObject,
subflags,
/* And add it to the pending-objects list */
if (numDependentObjects >= maxDependentObjects)
{
/* enlarge array if needed */
maxDependentObjects *= 2;
dependentObjects = (ObjectAddressAndFlags *)
repalloc(dependentObjects,
maxDependentObjects * sizeof(ObjectAddressAndFlags));
}
dependentObjects[numDependentObjects].obj = otherObject;
dependentObjects[numDependentObjects].subflags = subflags;
numDependentObjects++;
}
systable_endscan(scan);
/*
* Now we can sort the dependent objects into a stable visitation order.
* It's safe to use object_address_comparator here since the obj field is
* first within ObjectAddressAndFlags.
*/
if (numDependentObjects > 1)
qsort((void *) dependentObjects, numDependentObjects,
sizeof(ObjectAddressAndFlags),
object_address_comparator);
/*
* Now recurse to the dependent objects. We must visit them first since
* they have to be deleted before the current object.
*/
mystack.object = object; /* set up a new stack level */
mystack.flags = objflags;
mystack.next = stack;
for (int i = 0; i < numDependentObjects; i++)
{
ObjectAddressAndFlags *depObj = dependentObjects + i;
findDependentObjects(&depObj->obj,
depObj->subflags,
flags,
&mystack,
targetObjects,
@ -807,7 +864,7 @@ findDependentObjects(const ObjectAddress *object,
depRel);
}
systable_endscan(scan);
pfree(dependentObjects);
/*
* Finally, we can add the target object to targetObjects. Be careful to
@ -2109,18 +2166,31 @@ object_address_comparator(const void *a, const void *b)
const ObjectAddress *obja = (const ObjectAddress *) a;
const ObjectAddress *objb = (const ObjectAddress *) b;
/*
* Primary sort key is OID descending. Most of the time, this will result
* in putting newer objects before older ones, which is likely to be the
* right order to delete in.
*/
if (obja->objectId > objb->objectId)
return -1;
if (obja->objectId < objb->objectId)
return 1;
/*
* Next sort on catalog ID, in case identical OIDs appear in different
* catalogs. Sort direction is pretty arbitrary here.
*/
if (obja->classId < objb->classId)
return -1;
if (obja->classId > objb->classId)
return 1;
if (obja->objectId < objb->objectId)
return -1;
if (obja->objectId > objb->objectId)
return 1;
/*
* We sort the subId as an unsigned int so that 0 will come first. See
* logic in eliminate_duplicate_dependencies.
* Last, sort on object subId.
*
* We sort the subId as an unsigned int so that 0 (the whole object) will
* come first. This is essential for eliminate_duplicate_dependencies,
* and is also the best order for findDependentObjects.
*/
if ((unsigned int) obja->objectSubId < (unsigned int) objb->objectSubId)
return -1;

2
src/test/regress/expected/alter_table.out
View File

@ -2583,10 +2583,10 @@ DETAIL: drop cascades to table alter2.t1
drop cascades to view alter2.v1
drop cascades to function alter2.plus1(integer)
drop cascades to type alter2.posint
drop cascades to operator family alter2.ctype_hash_ops for access method hash
drop cascades to type alter2.ctype
drop cascades to function alter2.same(alter2.ctype,alter2.ctype)
drop cascades to operator alter2.=(alter2.ctype,alter2.ctype)
drop cascades to operator family alter2.ctype_hash_ops for access method hash
drop cascades to conversion alter2.ascii_to_utf8
drop cascades to text search parser alter2.prs
drop cascades to text search configuration alter2.cfg

8
src/test/regress/expected/create_type.out
View File

@ -161,13 +161,13 @@ DROP FUNCTION base_fn_out(opaque); -- error
ERROR: function base_fn_out(opaque) does not exist
DROP TYPE base_type; -- error
ERROR: cannot drop type base_type because other objects depend on it
DETAIL: function base_fn_out(base_type) depends on type base_type
function base_fn_in(cstring) depends on type base_type
DETAIL: function base_fn_in(cstring) depends on type base_type
function base_fn_out(base_type) depends on type base_type
HINT: Use DROP ... CASCADE to drop the dependent objects too.
DROP TYPE base_type CASCADE;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to function base_fn_out(base_type)
drop cascades to function base_fn_in(cstring)
DETAIL: drop cascades to function base_fn_in(cstring)
drop cascades to function base_fn_out(base_type)
-- Check usage of typmod with a user-defined type
-- (we have borrowed numeric's typmod functions)
CREATE TEMP TABLE mytab (foo widget(42,13,7)); -- should fail

4
src/test/regress/expected/domain.out
View File

@ -645,8 +645,8 @@ alter domain dnotnulltest drop not null;
update domnotnull set col1 = null;
drop domain dnotnulltest cascade;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to column col1 of table domnotnull
drop cascades to column col2 of table domnotnull
DETAIL: drop cascades to column col2 of table domnotnull
drop cascades to column col1 of table domnotnull
-- Test ALTER DOMAIN .. DEFAULT ..
create table domdeftest (col1 ddef1);
insert into domdeftest default values;

8
src/test/regress/expected/matview.out
View File

@ -311,12 +311,12 @@ SELECT type, m.totamt AS mtot, v.totamt AS vtot FROM mvtest_tm m LEFT JOIN mvtes
DROP TABLE mvtest_t;
ERROR: cannot drop table mvtest_t because other objects depend on it
DETAIL: view mvtest_tv depends on table mvtest_t
materialized view mvtest_mvschema.mvtest_tvm depends on view mvtest_tv
materialized view mvtest_tvmm depends on materialized view mvtest_mvschema.mvtest_tvm
view mvtest_tvv depends on view mvtest_tv
materialized view mvtest_tvvm depends on view mvtest_tvv
view mvtest_tvvmv depends on materialized view mvtest_tvvm
materialized view mvtest_bb depends on view mvtest_tvvmv
materialized view mvtest_mvschema.mvtest_tvm depends on view mvtest_tv
materialized view mvtest_tvmm depends on materialized view mvtest_mvschema.mvtest_tvm
materialized view mvtest_tm depends on table mvtest_t
materialized view mvtest_tmm depends on materialized view mvtest_tm
HINT: Use DROP ... CASCADE to drop the dependent objects too.
@ -327,12 +327,12 @@ BEGIN;
DROP TABLE mvtest_t CASCADE;
NOTICE: drop cascades to 9 other objects
DETAIL: drop cascades to view mvtest_tv
drop cascades to materialized view mvtest_mvschema.mvtest_tvm
drop cascades to materialized view mvtest_tvmm
drop cascades to view mvtest_tvv
drop cascades to materialized view mvtest_tvvm
drop cascades to view mvtest_tvvmv
drop cascades to materialized view mvtest_bb
drop cascades to materialized view mvtest_mvschema.mvtest_tvm
drop cascades to materialized view mvtest_tvmm
drop cascades to materialized view mvtest_tm
drop cascades to materialized view mvtest_tmm
ROLLBACK;

4
src/test/regress/expected/updatable_views.out
View File

@ -334,6 +334,7 @@ DETAIL: drop cascades to view ro_view1
drop cascades to view ro_view17
drop cascades to view ro_view2
drop cascades to view ro_view3
drop cascades to view ro_view4
drop cascades to view ro_view5
drop cascades to view ro_view6
drop cascades to view ro_view7
@ -341,11 +342,10 @@ drop cascades to view ro_view8
drop cascades to view ro_view9
drop cascades to view ro_view11
drop cascades to view ro_view13
drop cascades to view rw_view14
drop cascades to view rw_view15
drop cascades to view rw_view16
drop cascades to view ro_view20
drop cascades to view ro_view4
drop cascades to view rw_view14
DROP VIEW ro_view10, ro_view12, ro_view18;
DROP SEQUENCE uv_seq CASCADE;
NOTICE: drop cascades to view ro_view19