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

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> On Fri, Feb 8, 2019 at 8:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> * The other such issue is a pre-existing bug, which maybe we ought to
>> back-patch, though I can't recall seeing any field reports that seem
>> to match it: after recursing to an internal/extension dependency,
>> we need to ensure that whatever objflags were passed down to our level
>> get applied to the targetObjects entry for the current object.

> Hmm. This seems very subtle to me. Perhaps the comment you've added
> above the new object_address_present_add_flags() call in
> findDependentObjects() ought to explain the "current object gets
> marked with objflags" issue first, while only then mentioning the
> cross-check. The interface that object_address_present_add_flags()
> presents seems kind of odd to me, though I don't doubt that it makes
> sense in the wider context of the code.

How about this comment text?

/*
* The current target object should have been added to
* targetObjects while processing the owning object; but it
* probably got only the flag bits associated with the
* dependency we're looking at. We need to add the objflags
* that were passed to this recursion level, too, else we may
* get a bogus failure in reportDependentObjects (if, for
* example, we were called due to a partition dependency).
*
* If somehow the current object didn't get scheduled for
* deletion, bleat. (That would imply that somebody deleted
* this dependency record before the recursion got to it.)
* Another idea would be to reacquire lock on the current
* object and resume trying to delete it, but it seems not
* worth dealing with the race conditions inherent in that.
*/

>> [ invent separate primary and secondary partition dependencies? ]

> I lean towards changing these on HEAD, ...

Me too.

> ... now that it's clear that
> something very different will be needed for v11.

Just to be be clear, my inclination is to do nothing about this in v11.
It's not apparent to me that any fix is possible given the v11 dependency
data, at least not without downsides that'd likely outweigh the upsides.
We've not seen field complaints about these problems.

> That said, I still don't think that partition_dependency_matches() is
> all that bad, since the state is still right there in the pg_depend
> entry. The main drawback of that overall approach is that it obscures
> a legitimate distinction about dependencies that could be made more
> apparent to somebody looking through raw pg_depend entries.

The thing I don't like about it is that it's not very hard to foresee
cases where the approach will fail to uniquely resolve the primary
dependency. Making the original creator of the dependencies specify
which object is the primary owner seems a lot more future-proof.

regards, tom lane