Fixing findDependentObjects()'s dependency on scan order (regressions in DROP diagnostic messages)

From: Peter Geoghegan <pg(at)bowt(dot)ie>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Fixing findDependentObjects()'s dependency on scan order (regressions in DROP diagnostic messages)
Date: 2018-11-06 00:04:04
Message-ID: CAH2-Wzkypv1R+teZrr71U23J578NnTBt2X8+Y=Odr4pOdW1rXg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I've realized that my patch to make nbtree keys unique by treating
heap TID as a tie-breaker attribute must use ASC ordering, for reasons
that I won't go into here. Now that I'm not using DESC ordering, there
are changes to a small number of DROP...CASCADE messages that leave
users with something much less useful than what they'll see today --
see attached patch for full details. Some of these problematic cases
involve partitioning:

"""
create table trigpart (a int, b int) partition by range (a);
create table trigpart1 partition of trigpart for values from (0) to (1000);
create trigger trg1 after insert on trigpart for each row execute
procedure trigger_nothing();
...
drop trigger trg1 on trigpart1; -- fail
-ERROR: cannot drop trigger trg1 on table trigpart1 because trigger
trg1 on table trigpart requires it
-HINT: You can drop trigger trg1 on table trigpart instead.
+ERROR: cannot drop trigger trg1 on table trigpart1 because table
trigpart1 requires it
+HINT: You can drop table trigpart1 instead.
"""

As you can see, the original hint suggests "you need to drop the
object on the partition parent instead of its child", which is useful.
The new hint suggests "instead of dropping the trigger on the
partition child, maybe drop the child itself!", which is less than
useless. This is a problem that needs to be treated as a prerequisite
to committing the nbtree patch, so I'd like to get it out of the way
soon.

The high level issue is that findDependentObjects() relies on the scan
order of duplicates within the
DependDependerIndexId/pg_depend_depender_index index in a way that
nbtree doesn't actually guarantee, and never has guaranteed. As I've
shown, findDependentObjects()'s assumptions around where nbtree will
leave duplicates accidentally affects the quality of various
diagnostic messages. My example also breaks with
ignore_system_indexes=on, even on the master branch, so technically
this isn't a new problem.

I've looked into a way to fix findDependentObjects(). As far as I can
tell, I can fix issues by adding a kludgey special case along these
lines:

1 diff --git a/src/backend/catalog/dependency.c
b/src/backend/catalog/dependency.c
2 index 7dfa3278a5..7454d4e6f8 100644
3 --- a/src/backend/catalog/dependency.c
4 +++ b/src/backend/catalog/dependency.c
5 @@ -605,6 +605,15 @@ findDependentObjects(const ObjectAddress *object,
6 ReleaseDeletionLock(object);
7 return;
8 }
9 + /*
10 + * Assume that another pg_depend entry more suitably
11 + * represents dependency when an entry for a partition
12 + * child's index references a column of the partition
13 + * itself.
14 + */
15 + if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO &&
16 + otherObject.objectSubId != 0)
17 + break;

This is obviously brittle, but maybe it hints at a better approach.
Notably, it doesn't fix other similar issues, such as this:

--- a/contrib/earthdistance/expected/earthdistance.out
+++ b/contrib/earthdistance/expected/earthdistance.out
@@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
'(0)'::cube) / earth() - 1) <

drop extension cube; -- fail, earthdistance requires it
ERROR: cannot drop extension cube because other objects depend on it
-DETAIL: extension earthdistance depends on extension cube
+DETAIL: extension earthdistance depends on function cube_out(cube)

Can anyone think of a workable, scalable approach to fixing the
processing order of this findDependentObjects() pg_depend scan so that
we reliably get the user-visible behavior we already tacitly expect?

--
Peter Geoghegan

Attachment Content-Type Size
0008-Questionable-tentative-ASC-regress-output-fixes.patch application/x-patch 7.8 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-11-06 00:20:26 Re: Getting ERROR: could not open file "base/13164/t3_16388" with partition table with ON COMMIT
Previous Message Tatsuo Ishii 2018-11-05 23:43:03 Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan,Re: Code of Conduct plan