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

I wrote:
> I believe that we need to establish the following principles:
>
> * The terminology "internal_auto" is disastrously unhelpful.
> I propose calling these things "partition" dependencies instead.
>
> * Partition dependencies are not singletons. They should *always*
> come in pairs, one on the parent partitioned object (partitioned
> index, constraint, trigger, etc) and one on the child partitioned
> table. (Have I mentioned that our partition/partitioned terminology
> is also a disaster?) Maybe someday there'll be a reason to have
> three or more of these for the same dependent object, but there's
> no reason to have just one --- you should use an internal dependency
> instead for that case.
>
> * Partition dependencies are made *in addition to*, not instead of,
> the dependencies the object would normally have. In this way,
> for example, the unlink action in IndexSetParentIndex would just
> delete the partition dependencies and not have to worry about putting
> back the index's usual dependencies. Consistent with that, it's
> simply wrong that index_create sometimes marks the "usual"
> dependencies as internal_auto rather than auto. (It wasn't even
> doing that consistently; expression and predicate column dependencies
> were still "auto", which makes no sense at all.) They should go
> back to being plain auto with the partition dependencies being added
> separately. That will work properly given the changes that say that
> arriving at a partition-dependent object via an auto dependency is
> not sufficient license to delete the object.

Here's a patch along these lines. Some notes worth making:

* After spending a fair amount of effort on the description of the
dependency types in catalogs.sgml, I decided to just rip out the
duplicative text in dependency.h in favor of a pointer to catalogs.sgml.
If anybody's really hot to have two copies, we could put that back, but
I don't see the point.

* The core idea of the changes in dependency.c is just to wait till the
end of the entire dependency tree traversal, and then (before we start
actually deleting anything) check to make sure that each targeted
partition-dependent object has been reached via a partition-type
dependency, implying that at least one of its partition owners was
deleted. The existing data structure handles this nicely, we just
need a couple more flag bits for the specific need. (BTW, I experimented
with whether we could handle internal dependencies the same way; but
it didn't work, because of the previously-poorly-documented fact that
an internal dependency is immediately turned into a reverse-direction
recursion. We can't really muck with the dependency traversal order,
or we find ourselves deleting tables before their indices, etc.)

* I found two different ways in which dependency.c was still producing
traversal-order-sensitive results. One we already understood is that the
first loop in findDependentObjects threw an error for the first internal
or partition dependency it came across; since an object can have both of
those, the results varied. This was fixed by postponing the actual error
report to after the loop and adding an explicit preference order for what
to report.

* 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. Otherwise
the final state of the entry can vary depending on traversal order
(since orders in which the current call sees the object already in
targetObjects, or on the stack, would apply the objflags). This also
provides a chance to verify, not just assume, that processing of the
internal/extension dependency deleted the current object. As I mentioned
the other day, failing to ensure that the current object gets deleted
is very bad, and the pg_depend network is no longer so immutable that
we really ought to just assume that control came back to our object.

* The changes outside dependency.c just amount to applying the rules
stated above about how to use partition dependencies. I reverted the
places where v11 had decided that partition dependencies could be
substituted for auto dependencies, and made sure they are always
created in pairs.

There are a couple of things I didn't do here because those parts
of the patch were written while I still had some hope of applying
it to v11. Given that a catversion bump is needed anyway due to
the changes in what pg_depend entries are created for partitions,
we could change these:

* I renamed the dependency type to DEPENDENCY_PARTITION internally,
but left its pg_depend representation as 'I'. In a green field
we'd probably have made it 'P' instead.

* We could get rid of partition_dependency_matches(), which is not
really anything but a kluge, by splitting DEPENDENCY_PARTITION
into two dependency types, say DEPENDENCY_PARTITION_PRI ('P')
and DEPENDENCY_PARTITION_SEC ('S').

The main argument against changing these would be the risk that
client code already knows about 'I'. But neither pg_dump nor psql
do, so I judge that probably there's little if anything out there
that is special-casing that dependency type.

Also, I came across some coding in CloneFkReferencing() that looks fishy
as hell: that function imagines that it can delete an existing trigger
with nothing more than a summary CatalogTupleDelete(). I didn't do
anything about that here, but if it's not broken, I'd like to see an
explanation why not. I added a comment complaining about the lack of
pg_depend cleanup, and there's also the question of whether we don't
need to broadcast a relcache inval for the trigger's table.

regards, tom lane