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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> On 2019-Jan-18, Peter Geoghegan wrote:
>> I thought that your ALTER OBJECT DEPENDS ON EXTENSION example made the
>> case for fixing that directly inarguable. I'm slightly surprised that
>> you're not fully convinced of this already. Have I missed some
>> subtlety?

> I agree that it needs fixed, but I don't think we know what to change it
> *to*. The suggestion to use one AUTO and one INTERNAL seems to me to
> break some use cases. Maybe one INTERNAL and one INTERNAL_AUTO works
> well, not sure.

I've been chewing on this problem off-list, and frankly it's a mess.

The closest I could get to solving it along the original lines
went like this:

* In addition, we support INTERNAL_AUTO dependencies, which alter the
* rules for this. If the target has both INTERNAL and INTERNAL_AUTO
* dependencies, then it can be dropped if any one of those objects is
* dropped, but not unless at least one of them is. In this situation we
* mustn't automatically transform a random deletion request into a
* parent-object deletion. Instead, we proceed with deletion if we are
* recursing from any owning object, and otherwise set the object aside to
* recheck at the end. If we don't later find a reason to delete one of
* the owning objects, we'll throw an error.

I think that's ugly as sin: INTERNAL means something different if there's
another dependency beside it? It also turns out to have some
implementation problems we need not get into here, but which would create
a performance penalty for deletions.

A theoretically purer answer is to split INTERNAL_AUTO into two new
dependency types, one linking to the real "owning" object (the parent
index or trigger) and the other to the secondary owner (the partition
table), while leaving regular INTERNAL as it stands. I don't especially
want to go that way, though: it seems overcomplicated from the standpoint
of outside inspections of pg_depend, and it'd be very risky to back-patch
into v11. (For instance, if someone went backwards on their postmaster
minor release, the older v11 version would not know what to do with the
new dependency type.)

It strikes me however that we can stick with the existing catalog contents
by making this definition: of the INTERNAL_AUTO dependencies of a given
object, the "true owner" to be reported in errors is the dependency
that is of the same classId as that object. If this doesn't uniquely
identify one dependency, the report will mention a random one. This is
ugly as well, but it's certainly a lot more practical to cram into v11,
and it seems like it would cover the current and likely future use-cases
for partition-related objects. When and if we find a use-case for which
this doesn't work, we can think about extending the catalog representation
to identify a unique owner in a less ad-hoc way.

With either of these, the implementation I envision is to let the first
scan loop in findDependentObjects examine all the ref dependencies before
deciding what to do. If any one of the INTERNAL_AUTO dependencies is
being recursed from, we can allow the deletion to proceed. If not, do not
continue with the deletion, but save the target object's ID into a side
array, along with the ID of the object we determined to be its "true
owner". After completing the recursive dependency-tree walk, check each
object listed in the side array to see if it got deleted (by looking in
the main array of deletable objects that findDependentObjects reports).
If not, complain, citing the also-saved owning object ID as the object to
delete instead. This closes the hole I identified that the existing code
just assumes it can skip deleting the dependent object.

There are still some subtleties here: even if we didn't delete the
dependent object, someone else might've done so concurrently, which'd
result in getObjectDescription() failing if we just blindly try to
print an error message. We could fix that by re-locking the object
and seeing if it still exists before complaining; but I wonder if that
raises the risks of deadlocks materially.

regards, tom lane