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

On Fri, Feb 8, 2019 at 8:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > * 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.

> > * Partition dependencies are made *in addition to*, not instead of,
> > the dependencies the object would normally have.

> Here's a patch along these lines.

Formality: Unsurprisingly, your patch removes any need for the nbtree
patch series to paper-over test failures in indexing.out and
triggers.out, without creating any new issues with the regression
tests (a tiny number of harmless noise changes are needed -- no change
there). I can confirm that your patch fixes remaining regression test
breakage of the general variety described in my original e-mail of
November 5th. Once you commit this patch, I won't need to include any
kind of wonky workaround with each new revision of the patch series.

Many thanks for your help here.

> * 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.

+1 -- I see no need for repetition. In general, the pg_depend docs
seem easier to follow now.

> * 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.

Makes sense.

> * 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).

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.

> * 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.

Makes sense. It's inherently necessary for code outside of
dependency.c to get it right.

> 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.

I lean towards changing these on HEAD, now that it's clear that
something very different will be needed for v11. I agree that the
"internal_auto" terminology is very unhelpful, and it seems like a
good idea to fully acknowledge that certain dependency graphs have
qualities that are best thought of as peculiar to partitioning. In the
unlikely event that this did end up breaking external client code that
relies on the old constants, then the client code was probably subtly
wrong to begin with. This may be one of those cases where breaking
client code in a noticeable way is desirable.

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.

--
Peter Geoghegan