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

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> On Thu, Jan 17, 2019 at 4:40 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> We're going to stick all these items into an ObjectAddress array anyway,
>> so at worst it'd be 2X growth, most likely a lot less since we'd only
>> be sorting one level of dependency at a time.

> It sounds like we don't have a good reason to not just sort them
> explicitly, then. I'm happy to go that way. I mostly just wanted to be
> sure that you were aware that we could add a tie-breaker column
> without any storage overhead.

I think the tiebreaker idea is just a hack, because it'd only be stable
to the extent that the added tiebreaker values are stable, and they
wouldn't be very much so if the counter state is only kept per-backend.

Attached is a draft patch to sort objects before the recursion step
in findDependentObjects. I found that sorting by descending OID is
really the right thing; if we sort by increasing OID then we get a
whole lot more diffs in the DROP CASCADE output. As shown, there
are just a few such diffs, and many of them seem to be for the better
anyway.

I repurposed object_address_comparator for this, which means this
has a side-effect of changing the order in which pg_depend entries
for a new object are inserted into that catalog. I don't think
this is an issue.

I did not do anything here about reverting the various hacks to
suppress DROP CASCADE printouts in specific regression tests.
I'm not very sure whether doing so would be useful or not.

Testing this under ignore_system_indexes, it fixes most of the
cases where the output changes, but there are still two categories
it doesn't fix:

* Objects-to-drop output from DROP ROLE is still unstable. I suppose
this would be fixed by also doing sorting in that code path, but
I've not looked into it.

* There is still instability in which object you get told to drop
when attempting to drop an index partition or trigger, as a consequence
of there being two possible DEPENDENCY_INTERNAL_AUTO targets. I still
feel that the right fix there involves changing the design for what
dependency types we store, but I've not worked on it yet.

Comments?

regards, tom lane