Horton

Thread

Commits

  1. Redesign the partition dependency mechanism.

  2. Fix trigger drop procedure

  3. Sort the dependent objects before recursing in findDependentObjects().

  4. Avoid sometimes printing both tables and their columns in DROP CASCADE.

  1. Fixing findDependentObjects()'s dependency on scan order (regressions in DROP diagnostic messages)

    Peter Geoghegan <pg@bowt.ie> — 2018-11-06T00:04:04Z

    I've realized that my patch to make nbtree keys unique by treating
heap TID as a tie-breaker attribute must use ASC ordering, for reasons
that I won't go into here. Now that I'm not using DESC ordering, there
are changes to a small number of DROP...CASCADE messages that leave
users with something much less useful than what they'll see today --
see attached patch for full details. Some of these problematic cases
involve partitioning:

"""
 create table trigpart (a int, b int) partition by range (a);
 create table trigpart1 partition of trigpart for values from (0) to (1000);
 create trigger trg1 after insert on trigpart for each row execute
procedure trigger_nothing();
 ...
 drop trigger trg1 on trigpart1; -- fail
-ERROR:  cannot drop trigger trg1 on table trigpart1 because trigger
trg1 on table trigpart requires it
-HINT:  You can drop trigger trg1 on table trigpart instead.
+ERROR:  cannot drop trigger trg1 on table trigpart1 because table
trigpart1 requires it
+HINT:  You can drop table trigpart1 instead.
"""

As you can see, the original hint suggests "you need to drop the
object on the partition parent instead of its child", which is useful.
The new hint suggests "instead of dropping the trigger on the
partition child, maybe drop the child itself!", which is less than
useless. This is a problem that needs to be treated as a prerequisite
to committing the nbtree patch, so I'd like to get it out of the way
soon.

The high level issue is that findDependentObjects() relies on the scan
order of duplicates within the
DependDependerIndexId/pg_depend_depender_index index in a way that
nbtree doesn't actually guarantee, and never has guaranteed. As I've
shown, findDependentObjects()'s assumptions around where nbtree will
leave duplicates accidentally affects the quality of various
diagnostic messages. My example also breaks with
ignore_system_indexes=on, even on the master branch, so technically
this isn't a new problem.

I've looked into a way to fix findDependentObjects(). As far as I can
tell, I can fix issues by adding a kludgey special case along these
lines:

  1 diff --git a/src/backend/catalog/dependency.c
b/src/backend/catalog/dependency.c
  2 index 7dfa3278a5..7454d4e6f8 100644
  3 --- a/src/backend/catalog/dependency.c
  4 +++ b/src/backend/catalog/dependency.c
  5 @@ -605,6 +605,15 @@ findDependentObjects(const ObjectAddress *object,
  6                          ReleaseDeletionLock(object);
  7                          return;
  8                      }
  9 +                    /*
 10 +                     * Assume that another pg_depend entry more suitably
 11 +                     * represents dependency when an entry for a partition
 12 +                     * child's index references a column of the partition
 13 +                     * itself.
 14 +                     */
 15 +                    if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO &&
 16 +                        otherObject.objectSubId != 0)
 17 +                        break;

This is obviously brittle, but maybe it hints at a better approach.
Notably, it doesn't fix other similar issues, such as this:

--- a/contrib/earthdistance/expected/earthdistance.out
+++ b/contrib/earthdistance/expected/earthdistance.out
@@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
'(0)'::cube) / earth() - 1) <

 drop extension cube;  -- fail, earthdistance requires it
 ERROR:  cannot drop extension cube because other objects depend on it
-DETAIL:  extension earthdistance depends on extension cube
+DETAIL:  extension earthdistance depends on function cube_out(cube)

Can anyone think of a workable, scalable approach to fixing the
processing order of this findDependentObjects() pg_depend scan so that
we reliably get the user-visible behavior we already tacitly expect?

--
Peter Geoghegan

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

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2018-11-06T03:46:17Z

    In my opinion, your patch detected three problems:
1. Unsteady order of query results/system messages ('DROP...CASCADE' 
detects it).
2. Hide info about a child object dropping ('drop cascades to 62
other objects' detects it).
3. Possible non-informative messages about dependencies ('drop trigger 
trg1' detects it)

Problem No. 1 will be amplified with new asynchronous operations, 
background workers and distributing query execution. It is not problem 
of DBMS. The solution is change the tests: include sorting of query 
results, sorting of system messages before diff operation.
If steady order of messages is critical for users we can sort 
targetObjects array in the begin of reportDependentObjects() routine by 
classId, objectId and objectSubId fields.

Problem No. 2: we can suppress some output optimizations in 
object_address_present_add_flags() routine and print all deleted objects.

Problem No. 3: I suppose we can go one of two ways:
a) print all depended objects returned by scan of DependDependerIndexId 
relation, not only the first.
b) search a root of dependence and print only it.

On 06.11.2018 5:04, Peter Geoghegan wrote:
> I've realized that my patch to make nbtree keys unique by treating
> heap TID as a tie-breaker attribute must use ASC ordering, for reasons
> that I won't go into here. Now that I'm not using DESC ordering, there
> are changes to a small number of DROP...CASCADE messages that leave
> users with something much less useful than what they'll see today --
> see attached patch for full details. Some of these problematic cases
> involve partitioning:
> 
> """
>   create table trigpart (a int, b int) partition by range (a);
>   create table trigpart1 partition of trigpart for values from (0) to (1000);
>   create trigger trg1 after insert on trigpart for each row execute
> procedure trigger_nothing();
>   ...
>   drop trigger trg1 on trigpart1; -- fail
> -ERROR:  cannot drop trigger trg1 on table trigpart1 because trigger
> trg1 on table trigpart requires it
> -HINT:  You can drop trigger trg1 on table trigpart instead.
> +ERROR:  cannot drop trigger trg1 on table trigpart1 because table
> trigpart1 requires it
> +HINT:  You can drop table trigpart1 instead.
> """
> 
> As you can see, the original hint suggests "you need to drop the
> object on the partition parent instead of its child", which is useful.
> The new hint suggests "instead of dropping the trigger on the
> partition child, maybe drop the child itself!", which is less than
> useless. This is a problem that needs to be treated as a prerequisite
> to committing the nbtree patch, so I'd like to get it out of the way
> soon.
> 
> The high level issue is that findDependentObjects() relies on the scan
> order of duplicates within the
> DependDependerIndexId/pg_depend_depender_index index in a way that
> nbtree doesn't actually guarantee, and never has guaranteed. As I've
> shown, findDependentObjects()'s assumptions around where nbtree will
> leave duplicates accidentally affects the quality of various
> diagnostic messages. My example also breaks with
> ignore_system_indexes=on, even on the master branch, so technically
> this isn't a new problem.
> 
> I've looked into a way to fix findDependentObjects(). As far as I can
> tell, I can fix issues by adding a kludgey special case along these
> lines:
> 
>    1 diff --git a/src/backend/catalog/dependency.c
> b/src/backend/catalog/dependency.c
>    2 index 7dfa3278a5..7454d4e6f8 100644
>    3 --- a/src/backend/catalog/dependency.c
>    4 +++ b/src/backend/catalog/dependency.c
>    5 @@ -605,6 +605,15 @@ findDependentObjects(const ObjectAddress *object,
>    6                          ReleaseDeletionLock(object);
>    7                          return;
>    8                      }
>    9 +                    /*
>   10 +                     * Assume that another pg_depend entry more suitably
>   11 +                     * represents dependency when an entry for a partition
>   12 +                     * child's index references a column of the partition
>   13 +                     * itself.
>   14 +                     */
>   15 +                    if (foundDep->deptype == DEPENDENCY_INTERNAL_AUTO &&
>   16 +                        otherObject.objectSubId != 0)
>   17 +                        break;
> 
> This is obviously brittle, but maybe it hints at a better approach.
> Notably, it doesn't fix other similar issues, such as this:
> 
> --- a/contrib/earthdistance/expected/earthdistance.out
> +++ b/contrib/earthdistance/expected/earthdistance.out
> @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
> '(0)'::cube) / earth() - 1) <
> 
>   drop extension cube;  -- fail, earthdistance requires it
>   ERROR:  cannot drop extension cube because other objects depend on it
> -DETAIL:  extension earthdistance depends on extension cube
> +DETAIL:  extension earthdistance depends on function cube_out(cube)
> 
> Can anyone think of a workable, scalable approach to fixing the
> processing order of this findDependentObjects() pg_depend scan so that
> we reliably get the user-visible behavior we already tacitly expect?
> 
> --
> Peter Geoghegan
> 

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

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

    Peter Geoghegan <pg@bowt.ie> — 2018-11-13T21:29:25Z

    On Mon, Nov 5, 2018 at 7:46 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> Problem No. 1 will be amplified with new asynchronous operations,
> background workers and distributing query execution. It is not problem
> of DBMS. The solution is change the tests: include sorting of query
> results, sorting of system messages before diff operation.
> If steady order of messages is critical for users we can sort
> targetObjects array in the begin of reportDependentObjects() routine by
> classId, objectId and objectSubId fields.

Yeah, maybe. I'm not that worried about the order of objects; we can
probably continue to get away with suppressing the list of objects by
placing a "\set VERBOSITY terse" where needed -- that's something
we've already been doing for some time [1]. I accept that it would be
better to sort the output, but I'm concerned that that would be a
difficult, risky project. What if there was a huge number of dependent
objects? What if a memory allocation fails?

> Problem No. 2: we can suppress some output optimizations in
> object_address_present_add_flags() routine and print all deleted objects.

If there is anything that makes it necessary to sort, it's this -- the
order of visitation can affect whether or not
object_address_present_add_flags() suppresses redundant entries. But I
still prefer to fix the problem by changing the scan order to be what
we actually want it to be.

> Problem No. 3: I suppose we can go one of two ways:
> a) print all depended objects returned by scan of DependDependerIndexId
> relation, not only the first.
> b) search a root of dependence and print only it.

A solution does occur to me that I'm kind of embarrassed to suggest,
but that would nonetheless probably do the job:

The general problem here is that the scan order of a scan that uses
pg_depend_depender_index doesn't reliably give us the order we
actually want among duplicate index entries (at least, you could
choose to characterize it in that narrow way). The index
pg_depend_depender_index looks like this:

  Column  │  Type   │ Key? │ Definition
──────────┼─────────┼──────┼────────────
 classid  │ oid     │ yes  │ classid
 objid    │ oid     │ yes  │ objid
 objsubid │ integer │ yes  │ objsubid
btree, for table "pg_catalog.pg_depend"

Note that this isn't a unique index -- there are no unique indexes on
pg_depend at all, in fact. (Note also that the same observations apply
to pg_shdepend.)

The objsubid (not refobjsubid) is 0 in all cases that we have problems
with -- we're never going to have a problem with multiple
dependent-object-is-column entries that have the same '(classid,
objid, objsubid)' value, as far as I can tell:

pg@regression[2201]=# SELECT count(*), classid, objid, objsubid FROM
pg_depend WHERE objsubid != 0 GROUP BY classid, objid, objsubid HAVING
count(*) > 1;
 count │ classid │ objid │ objsubid
───────┼─────────┼───────┼──────────
(0 rows)

(i.e. We're only having problems with some of the entries/values that
you'll see when the above query is changed to have "bjsubid = 0" in
its WHERE clause.)

Why not vary the objsubid value among entries that don't use it
anyway, so that they have a negative integer objsubid values rather
than 0? That would have the effect of forcing the scan order of
pg_depend_depender_index scans to be whatever we want it to be.
dependent-object-is-column entries would not have their sort order
affected anyway, because in practice there is only one entry with the
same  '(classid, objid, objsubid)' value when objsubid > 0.

We could invent some scheme that made pg_depend/pg_shdepend entries
use fixed objsubid negative integer codes according to the
*referenced* object type (refobjid), and/or the entry's deptype.
Places that look at ObjectAddress.objectSubId would look for entries
that were > 0 rather than != 0, so we wouldn't really be changing the
meaning of objsubid (though it would need to be documented in the user
docs, and have a release note compatibility entry).
findDependentObjects() would still use "nkeys = 2" for these entries;
it would be one of the places that would be taught to check
objectSubId > 0 rather than objectSubId != 0. But it would get the
scan order it actually needs.

I haven't attempted to put this together just yet, because I want to
see if it passes the laugh test. My sense is that the best way to fix
the problem is to force the scan order to be the one that we
accidentally depend on using some mechanism or other. Though not
necessarily the one I've sketched out.

[1] https://postgr.es/m/24279.1525401104@sss.pgh.pa.us
--
Peter Geoghegan

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

    Peter Geoghegan <pg@bowt.ie> — 2018-11-14T06:28:05Z

    On Tue, Nov 13, 2018 at 1:29 PM Peter Geoghegan <pg@bowt.ie> wrote:
> A solution does occur to me that I'm kind of embarrassed to suggest,
> but that would nonetheless probably do the job:

> Why not vary the objsubid value among entries that don't use it
> anyway, so that they have a negative integer objsubid values rather
> than 0? That would have the effect of forcing the scan order of
> pg_depend_depender_index scans to be whatever we want it to be.

I tried to implement this today, and it almost worked. However, what I
came up with was even more brittle than I thought it would be, because
knowledge of objsubid is all over the place. It's even in pg_dump.

Then I had a better idea: add a monotonically decreasing column to
pg_depend, and make it the last attribute in both
pg_depend_depender_index and pg_depend_reference_index (it has to be
both indexes). This makes almost all the regression test output go
back to the master branch output, since index-wise duplicates are now
accessed in a well defined order: reverse chronological order (the
reverse of the order of the creation of a pg_depend entry). There is a
tiny amount of stuff that still has a different order in the
regression test output, but the changes are trivial, and obviously
still correct.

Is this approach really so bad? I might want to find a way to make it
a monotonically increasing DESC column (there is a question around bki
grammar support for that), but that's just a detail. Some scheme
involving a new column seems like it will work reasonably well. It's
much more maintainable than anything else I'm likely to come up with.

The indexes are actually smaller with the change following a run of
the regression tests, provided the entire patch series is applied. So
even if the only thing you care about is system catalog size, you
still win (the table is slightly bigger, though). This approach will
fix most or all of the test flappiness that we have been papering over
with "\set VERBOSITY terse" before now. I can't promise that just
making this one pg_depend change will reliably make that class of
problem go away forever, since you could still see something like that
elsewhere, but it looks like the vast majority of problem cases
involve pg_depend. So we might just end up killing the "\set VERBOSITY
terse" hack completely this way.

We're already relying on the scan order being in reverse chronological
order, so we might as well formalize the dependency. I don't think
that it's possible to sort the pg_depend entries as a way of fixing
the breakage while avoiding storing this extra information -- what is
there to sort on that's there already? You'd have to infer a whole
bunch of things about the object types associated with pg_depend
entries to do that, and teach dependency.c about its callers. That
seems pretty brittle to me.

-- 
Peter Geoghegan

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

    Robert Haas <robertmhaas@gmail.com> — 2018-11-15T18:52:29Z

    On Wed, Nov 14, 2018 at 1:28 AM Peter Geoghegan <pg@bowt.ie> wrote:
> We're already relying on the scan order being in reverse chronological
> order, so we might as well formalize the dependency. I don't think
> that it's possible to sort the pg_depend entries as a way of fixing
> the breakage while avoiding storing this extra information -- what is
> there to sort on that's there already? You'd have to infer a whole
> bunch of things about the object types associated with pg_depend
> entries to do that, and teach dependency.c about its callers. That
> seems pretty brittle to me.

I think that the solution that you are proposing sorta sucks, but it's
better than hacking objsubid to do it, which did in fact pass the
laugh test, in that I laughed when I read it.  :-)

In a perfect world, it seems to me that what we ought to do is have
some real logic in the server that figures out which of the various
things we could report would be most likely to be useful to the user
... but that's probably a non-trivial project involving a fair amount
of human judgement.  Reasonable people may differ about what is best,
never mind unreasonable people.  I am inclined to think that your
proposal here is good enough for now, and somebody who dislikes it
(surely such a person will exist) can decide to look for ways to make
it better.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

    Peter Geoghegan <pg@bowt.ie> — 2018-11-15T19:19:05Z

    On Thu, Nov 15, 2018 at 10:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I think that the solution that you are proposing sorta sucks, but it's
> better than hacking objsubid to do it, which did in fact pass the
> laugh test, in that I laughed when I read it.  :-)

Probably a good idea to get another cup of coffee if I'm
pre-apologizing for my ideas.

> In a perfect world, it seems to me that what we ought to do is have
> some real logic in the server that figures out which of the various
> things we could report would be most likely to be useful to the user
> ... but that's probably a non-trivial project involving a fair amount
> of human judgement.  Reasonable people may differ about what is best,
> never mind unreasonable people.  I am inclined to think that your
> proposal here is good enough for now, and somebody who dislikes it
> (surely such a person will exist) can decide to look for ways to make
> it better.

Great. Actually, the on-disk size of the pg_depend heap relation is
*unchanged* in the attached WIP patch, since it fits in a hole
previously lost to alignment. And, as I said, the indexes end up
smaller with suffix truncation. Even if the only thing you care about
is the on-disk size of system catalogs, you'll still pretty reliably
come out ahead. The design here is squirrelly, but we're already
relying on scan order to reach objsubid = 0 entries first.

There is a single tiny behavioral change to the regression test output
with this patch applied. I think that that's just because there is one
place where this dependency management stuff interacts with pages full
of duplicates, and therefore stops putting duplicates in pg_depend
indexes in exactly DESC TID order. My other patches add a couple of
more tiny changes along similar lines, since of course I'm only doing
this with the pg_depend indexes, and not for every system catalog
index.

-- 
Peter Geoghegan

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

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2018-12-06T06:35:29Z

    On 14.11.2018 11:28, Peter Geoghegan wrote:
> We're already relying on the scan order being in reverse chronological
> order, so we might as well formalize the dependency. I don't think
> that it's possible to sort the pg_depend entries as a way of fixing
> the breakage while avoiding storing this extra information -- what is
> there to sort on that's there already? You'd have to infer a whole
> bunch of things about the object types associated with pg_depend
> entries to do that, and teach dependency.c about its callers. That
> seems pretty brittle to me.

This solution changes pg_depend relation for solve a problem, which 
exists only in regression tests.  Very rarely it can be in the 
partitioning cases. Or is it not?
I think this decision is some excessive.
May be you consider another approach:
1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test 
tools, not DBMS. And here we can use 'verbose terse'.
2. Print all dependencies in findDependentObjects() on a drop error (see 
attachment as a prototype).

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-06T06:52:30Z

    On Wed, Dec 5, 2018 at 10:35 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> This solution changes pg_depend relation for solve a problem, which
> exists only in regression tests.  Very rarely it can be in the
> partitioning cases. Or is it not?

I don't think it's a matter of how rarely this will happen. We're
trying to avoid these diagnostic message changes because they are
wrong. I don't think that there is much ambiguity about that.  Still,
it will happen however often the user drops objects belonging to
partition children, which could be quite often.

> I think this decision is some excessive.
> May be you consider another approach:
> 1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test
> tools, not DBMS. And here we can use 'verbose terse'.
> 2. Print all dependencies in findDependentObjects() on a drop error (see
> attachment as a prototype).

You didn't include changes to the regression test output, which seems
like a big oversight, given that this has a lot to do with diagnostic
messages that are represented in the regression tests. Anyway, I don't
think it's acceptable to change the messages like this. It makes them
much less useful.

These stability issue keeps coming up, which makes a comprehensive
approach seem attractive to me. At least 95% of the test instability
comes from pg_depend.
-- 
Peter Geoghegan

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

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2018-12-07T04:52:12Z

    
On 06.12.2018 11:52, Peter Geoghegan wrote:
> On Wed, Dec 5, 2018 at 10:35 PM Andrey Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> This solution changes pg_depend relation for solve a problem, which
>> exists only in regression tests.  Very rarely it can be in the
>> partitioning cases. Or is it not?
> 
> I don't think it's a matter of how rarely this will happen. We're
> trying to avoid these diagnostic message changes because they are
> wrong. I don't think that there is much ambiguity about that.  Still,
> it will happen however often the user drops objects belonging to
> partition children, which could be quite often.

I want to say that we need to localize the rules for the order of the 
diagnostic messages as much as possible in dependency.c.
> 
>> I think this decision is some excessive.
>> May be you consider another approach:
>> 1. Order of dependencies in 'DROP ... CASCADE' case is a problem of test
>> tools, not DBMS. And here we can use 'verbose terse'.
>> 2. Print all dependencies in findDependentObjects() on a drop error (see
>> attachment as a prototype).
> 
> You didn't include changes to the regression test output, which seems
> like a big oversight...

It was done knowingly to show the differences in messages that 
introduces this approach.

> messages that are represented in the regression tests. Anyway, I don't
> think it's acceptable to change the messages like this. It makes them
> much less useful.

May you clarify this? If I understand correctly, your solution is that 
user receives a report about the last inserted internal dependency on 
the object. Why the full info about internal dependencies of the object 
much less useful?
> 
> These stability issue keeps coming up, which makes a comprehensive
> approach seem attractive to me. At least 95% of the test instability
> comes from pg_depend.
> 

During the retail index tuple deletion project (heap cleaner subsystem) 
we have non-fixed order of tuples at database relations. This caused to 
unsteady order of text strings in some check-world test results.
Thus, I realized that the order of messages in the test results is 
mostly a game of chance. For this reason I think it is necessary to find 
more general solution of the messages ordering problem.

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-08T01:58:19Z

    On Thu, Dec 6, 2018 at 8:52 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> I want to say that we need to localize the rules for the order of the
> diagnostic messages as much as possible in dependency.c.

But the issue *isn't* confined to dependency.c, anyway. It bleeds into
a couple of other modules, like extension.c and tablecmds.c.

> > messages that are represented in the regression tests. Anyway, I don't
> > think it's acceptable to change the messages like this. It makes them
> > much less useful.
>
> May you clarify this? If I understand correctly, your solution is that
> user receives a report about the last inserted internal dependency on
> the object. Why the full info about internal dependencies of the object
> much less useful?

I don't know why for sure -- I just know that it is. It must have
evolved that way, as software often does. It's not surprising that the
most recently entered dependency is usually the most marginal among
dependencies of equal precedence in the real world, and therefore the
best suggestion. I've shown this with two examples.

To return to one of the examples: are you arguing that there is no
practical difference between "you need to drop the object on the
partition parent instead of its child" and "instead of dropping the
trigger on the partition child, maybe drop the child itself"? I think
it's *extremely* obvious that there is a big practical difference to
users in that particular case. Now, I agree that there are many other
examples where it doesn't really matter to users, but I don't think
that that's very relevant. Yes, these are the exceptions, but the
exceptions are often the interesting cases.

> During the retail index tuple deletion project (heap cleaner subsystem)
> we have non-fixed order of tuples at database relations. This caused to
> unsteady order of text strings in some check-world test results.
> Thus, I realized that the order of messages in the test results is
> mostly a game of chance. For this reason I think it is necessary to find
> more general solution of the messages ordering problem.

 I have no idea what you mean here. I'm proposing a patch that stops
it being a game of chance, while preserving the existing
slightly-random behavior to the greatest extent possible. I think that
my patch would have stopped that problem altogether. Are you
suggesting that it wouldn't have?

-- 
Peter Geoghegan

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

    Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2018-12-18T06:52:50Z

    
On 08.12.2018 6:58, Peter Geoghegan wrote:
>   I have no idea what you mean here. I'm proposing a patch that stops
> it being a game of chance, while preserving the existing
> slightly-random behavior to the greatest extent possible. I think that
> my patch would have stopped that problem altogether. Are you
> suggesting that it wouldn't have?

I did many tests of your solution inside the 'quick vacuum' strategy [1] 
and the background worker called 'heap cleaner' [2]. I must admit that 
when I  use your patch, there is no problem with dependencies.
This patch needs opinion of an another reviewer.

[1] 
https://www.postgresql.org/message-id/425db134-8bba-005c-b59d-56e50de3b41e%40postgrespro.ru
[2] 
https://www.postgresql.org/message-id/f49bb262-d246-829d-f835-3950ddac503c%40postgrespro.ru

-- 
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T06:56:56Z

    On Mon, Dec 17, 2018 at 10:52 PM Andrey Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> I did many tests of your solution inside the 'quick vacuum' strategy [1]
> and the background worker called 'heap cleaner' [2]. I must admit that
> when I  use your patch, there is no problem with dependencies.

Cool.

> This patch needs opinion of an another reviewer.

Was I unclear on why the patch fixes the problem? Sorry, but I thought
it was obvious.

-- 
Peter Geoghegan

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-18T18:07:07Z

    On 2018-Nov-05, Peter Geoghegan wrote:

> I've realized that my patch to make nbtree keys unique by treating
> heap TID as a tie-breaker attribute must use ASC ordering, for reasons
> that I won't go into here. Now that I'm not using DESC ordering, there
> are changes to a small number of DROP...CASCADE messages that leave
> users with something much less useful than what they'll see today --
> see attached patch for full details. Some of these problematic cases
> involve partitioning:

Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO
entries?  I wonder if I just haven't broken the algorithm when
introducing that, and I worry that we're adding a complicated kludge to
paper over that problem.  Maybe instead of the depcreate contortions we
need to adjust the algorithm to deal with INTERNAL_AUTO objects in a
different way.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-18T18:26:03Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Nov-05, Peter Geoghegan wrote:
>> I've realized that my patch to make nbtree keys unique by treating
>> heap TID as a tie-breaker attribute must use ASC ordering, for reasons
>> that I won't go into here. Now that I'm not using DESC ordering, there
>> are changes to a small number of DROP...CASCADE messages that leave
>> users with something much less useful than what they'll see today --
>> see attached patch for full details. Some of these problematic cases
>> involve partitioning:

> Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO
> entries?  I wonder if I just haven't broken the algorithm when
> introducing that, and I worry that we're adding a complicated kludge to
> paper over that problem.  Maybe instead of the depcreate contortions we
> need to adjust the algorithm to deal with INTERNAL_AUTO objects in a
> different way.

Yeah, I've been wondering about that as well.  The original intention
for dependency traversal was that it'd work independently of the ordering
of entries in pg_depend.  If it's not doing so, I'd call that a bug in
dependency traversal rather than something the index code needs to be
responsible for.

(Note that this statement doesn't disagree with our issues about needing
to suppress dependency reports in the regression tests; that's because
the order of reports about independent objects can legitimately depend on
the index order.  But there shouldn't be any semantic differences.)

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T18:34:17Z

    On Tue, Dec 18, 2018 at 10:07 AM Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Is there any case of this that doesn't involve DEPENDENCY_INTERNAL_AUTO
> entries?  I wonder if I just haven't broken the algorithm when
> introducing that, and I worry that we're adding a complicated kludge to
> paper over that problem.  Maybe instead of the depcreate contortions we
> need to adjust the algorithm to deal with INTERNAL_AUTO objects in a
> different way.

Well, you also have cases like this:

--- a/contrib/earthdistance/expected/earthdistance.out
+++ b/contrib/earthdistance/expected/earthdistance.out
@@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
'(0)'::cube) / earth() - 1) <

 drop extension cube;  -- fail, earthdistance requires it
 ERROR:  cannot drop extension cube because other objects depend on it
-DETAIL:  extension earthdistance depends on extension cube
+DETAIL:  extension earthdistance depends on function cube_out(cube)

This is a further example of "wrong, not just annoying". Technically
this is a broader problem than DEPENDENCY_INTERNAL_AUTO, I think,
though perhaps not too much broader.
-- 
Peter Geoghegan

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T18:46:59Z

    On Tue, Dec 18, 2018 at 10:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, I've been wondering about that as well.  The original intention
> for dependency traversal was that it'd work independently of the ordering
> of entries in pg_depend.  If it's not doing so, I'd call that a bug in
> dependency traversal rather than something the index code needs to be
> responsible for.

It's clearly not doing so. Is somebody else actually going to fix it,
though? I'm quite happy to stand aside to make way for a better
solution, but I don't consider myself particularly qualified to come
up with that better solution. The findDependentObjects() code is
pretty subtle.

> (Note that this statement doesn't disagree with our issues about needing
> to suppress dependency reports in the regression tests; that's because
> the order of reports about independent objects can legitimately depend on
> the index order.  But there shouldn't be any semantic differences.)

The advantage of my admittedly kludgy approach is that it will almost
completely eliminate any further need to suppress acceptable
regression test differences -- those non-semantic output differences
that we're already suppressing semi-regularly. In practice, adding a
trailing attribute to each of the two pg_depend indexes almost
entirely eliminates the need to play whack-a-mole. That has to be an
advantage for the kind of approach that I've suggested.

-- 
Peter Geoghegan

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-12-18T21:20:53Z

    On 2018-Dec-18, Peter Geoghegan wrote:

> Well, you also have cases like this:
> 
> --- a/contrib/earthdistance/expected/earthdistance.out
> +++ b/contrib/earthdistance/expected/earthdistance.out
> @@ -972,7 +972,7 @@ SELECT abs(cube_distance(ll_to_earth(-30,-90),
> '(0)'::cube) / earth() - 1) <
> 
>  drop extension cube;  -- fail, earthdistance requires it
>  ERROR:  cannot drop extension cube because other objects depend on it
> -DETAIL:  extension earthdistance depends on extension cube
> +DETAIL:  extension earthdistance depends on function cube_out(cube)
> 
> This is a further example of "wrong, not just annoying". Technically
> this is a broader problem than DEPENDENCY_INTERNAL_AUTO, I think,
> though perhaps not too much broader.

Hmm, interesting.  I wonder if this is just a case of never testing this
code under "postgres --ignore-system-indexes".  I can reproduce the
reported problem without your patch by using that flag.  Here's a
recipe:

create extension cube;
create table dep as select ctid as tid,* from pg_depend;
create extension earthdistance;
select tid, deptype, (dep).type, (dep).identity, (ref).type, (ref).identity
  from (select tid, deptype, pg_identify_object(classid, objid, objsubid) as dep,
               pg_identify_object(refclassid, refobjid, refobjsubid) as ref
          from (select ctid as tid, * from pg_depend except select * from dep) a
       ) b;

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T22:02:37Z

    On Tue, Dec 18, 2018 at 1:20 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2018-Dec-18, Peter Geoghegan wrote:
> Hmm, interesting.  I wonder if this is just a case of never testing this
> code under "postgres --ignore-system-indexes".

I suppose that you could say that. The regression tests will fail at
many points with --ignore-system-indexes, almost all of which are due
to well understood issues. For example, you'll get load of WARNINGs
about needing to use a system index despite the server being run under
--ignore-system-indexes.

> I can reproduce the
> reported problem without your patch by using that flag.  Here's a
> recipe:
>
> create extension cube;
> create table dep as select ctid as tid,* from pg_depend;
> create extension earthdistance;
> select tid, deptype, (dep).type, (dep).identity, (ref).type, (ref).identity
>   from (select tid, deptype, pg_identify_object(classid, objid, objsubid) as dep,
>                pg_identify_object(refclassid, refobjid, refobjsubid) as ref
>           from (select ctid as tid, * from pg_depend except select * from dep) a
>        ) b;

Interesting.

Note that if the standard that we're going to hold a solution to here
is "must produce sane output with  --ignore-system-indexes", then my
solution will not meet that standard. However, I fear that it's going
to be really hard to accomplish that goal some other way (besides
which, as I said, the tests will still fail with
--ignore-system-indexes for reasons that have nothing to do with
dependency management).

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-18T22:11:42Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 1:20 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> I can reproduce the
>> reported problem without your patch by using that flag.  Here's a
>> recipe:

> Interesting.
> Note that if the standard that we're going to hold a solution to here
> is "must produce sane output with  --ignore-system-indexes", then my
> solution will not meet that standard.

Do you mean "same" output, or "sane" output?  I'd certainly expect
the latter.

I think though that Alvaro was just offering this as a way to poke
at the posited bug in dependency.c without having to install your
whole patch.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T22:17:54Z

    On Tue, Dec 18, 2018 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Interesting.
> > Note that if the standard that we're going to hold a solution to here
> > is "must produce sane output with  --ignore-system-indexes", then my
> > solution will not meet that standard.
>
> Do you mean "same" output, or "sane" output?  I'd certainly expect
> the latter.

I meant sane.

--ignore-system-indexes leads to slightly wrong answers in a number of
the diagnostic messages run by the regression tests (I recall that the
number of objects affected by CASCADE sometimes differed, and I think
that there was also a certain amount of this DEPENDENCY_INTERNAL_AUTO
business that Alvaro looked into). I think that this must have always
been true.

My patch will not improve matters with --ignore-system-indexes. It
merely makes the currently expected output when scanning pg_depend
indexes occur reliably. For better or for worse.

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-12-18T22:26:24Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Do you mean "same" output, or "sane" output?  I'd certainly expect
>> the latter.

> I meant sane.

> --ignore-system-indexes leads to slightly wrong answers in a number of
> the diagnostic messages run by the regression tests (I recall that the
> number of objects affected by CASCADE sometimes differed, and I think
> that there was also a certain amount of this DEPENDENCY_INTERNAL_AUTO
> business that Alvaro looked into). I think that this must have always
> been true.

Hm, that definitely leads me to feel that we've got bug(s) in
dependency.c.  I'll take a look sometime soon.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2018-12-18T22:29:14Z

    On Tue, Dec 18, 2018 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hm, that definitely leads me to feel that we've got bug(s) in
> dependency.c.  I'll take a look sometime soon.

Thanks!

I'm greatly relieved that I probably won't have to become an expert on
dependency.c after all.   :-)
-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T20:42:00Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Dec 18, 2018 at 2:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm, that definitely leads me to feel that we've got bug(s) in
>> dependency.c.  I'll take a look sometime soon.

> Thanks!
> I'm greatly relieved that I probably won't have to become an expert on
> dependency.c after all.   :-)

So I poked around for awhile with running the regression tests under
ignore_system_indexes.  There seem to be a number of issues involved
here.  To a significant extent, they aren't bugs, at least not according
to the original conception of the dependency code: it was not a design
goal that different dependencies of the same object-to-be-deleted would
be processed in a fixed order.  That leads to the well-known behavior
that cascade drops report the dropped objects in an unstable order.

Now, perhaps we should make such stability a design goal, as it'd allow
us to get rid of some "suppress the cascade outputs" hacks in the
regression tests.  But it's a bit of a new feature.  If we wanted to
do that, I'd be inclined to do it by absorbing all the pg_depend entries
for a particular object into an ObjectAddress array and then sorting
them before we process them.  The main stumbling block here is "what
would the sort order be?".  The best idea I can come up with offhand
is to sort by OID, which at least for regression test purposes would
mean objects would be listed/processed more or less in creation order.

However, there are a couple of places where the ignore_system_indexes
output does something weird like

 DROP TABLE PKTABLE;
 ERROR:  cannot drop table pktable because other objects depend on it
-DETAIL:  constraint constrname2 on table fktable depends on table pktable
+DETAIL:  constraint constrname2 on table fktable depends on index pktable_pkey
 HINT:  Use DROP ... CASCADE to drop the dependent objects too.

The reason for this is that the reported "nearest dependency" is the
object that we first reached the named object by recursing from.
If there are multiple dependency paths to the same object then it's
order-traversal-dependent which path we take first.  Sorting the
pg_depend entries before processing, as I suggested above, would
remove the instability, but it's not real clear whether we'd get a
desirable choice of reported object that way.  Perhaps we could
improve matters by having the early exits from findDependentObjects
(at stack_address_present_add_flags and object_address_present_add_flags)
consider replacing the already-recorded dependee with the current
source object, according to some set of rules.  One rule that I think
would be useful is to compare dependency types: I think a normal
dependency is more interesting than an auto dependency which is
more interesting than an internal one.  Beyond that, though, I don't
see anything we could do but compare OIDs.  (If we do compare OIDs,
then the result should be stable with or without pre-sorting of the
pg_depend entries.)

I also noticed some places where the output reports a different number
of objects dropped by a cascade.  This happens when a table column
is reached by some dependency path, while the whole table is reached
by another one.  If we find the whole table first, then when we find
the table column we just ignore it.  But in the other order, they're
reported as two separate droppable objects.  The reasoning is
explained in object_address_present_add_flags:

                 * We get here if we find a need to delete a whole table after
                 * having already decided to drop one of its columns.  We
                 * can't report that the whole object is in the array, but we
                 * should mark the subobject with the whole object's flags.
                 *
                 * It might seem attractive to physically delete the column's
                 * array entry, or at least mark it as no longer needing
                 * separate deletion.  But that could lead to, e.g., dropping
                 * the column's datatype before we drop the table, which does
                 * not seem like a good idea.  This is a very rare situation
                 * in practice, so we just take the hit of doing a separate
                 * DROP COLUMN action even though we know we're gonna delete
                 * the table later.

I think this reasoning is sound, and we should continue to do the separate
DROP COLUMN step, but it occurs to me that just because we drop the column
separately doesn't mean we have to report it separately to the user.
I propose that we handle this case by adding a new DEPFLAG_IS_SUBOBJECT
flag to the column object's flags, denoting that we know the whole table
will be dropped later.  The only effect of this flag is to suppress
reporting of the column object in reportDependentObjects.

Another order-dependent effect that can be seen in the regression tests
comes from the fact that the first loop in findDependentObjects (over
the target's referenced objects) errors out immediately on the first
DEPENDENCY_INTERNAL, DEPENDENCY_INTERNAL_AUTO, or DEPENDENCY_EXTENSION
entry it finds.  When this code was written, that was fine because the
only possibility was DEPENDENCY_INTERNAL, and there can be only one
DEPENDENCY_INTERNAL dependency for an object.  The addition of
DEPENDENCY_EXTENSION makes that shaky: if you have a couple of
internally-related objects inside an extension, and you try to drop the
dependent one, in theory you might get told either to drop the extension
or to drop the parent object.  However, I believe we generally avoid
making DEPENDENCY_EXTENSION entries for internally-dependent objects
(they usually don't have their own owner dependencies either) so I'm not
sure the case arises in practice, or needs to arise in practice.  It'd
be reasonable to expect that any given object has at most one INTERNAL+
EXTENSION dependency.  (Whether it's reasonable to try to enforce that
is hard to say, though.)

DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
has no hesitation about making multiple entries of that kind.   After
rather cursorily looking at that code, I'm leaning to the position
that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
nuked from orbit.  In the cases where it's being used, such as
partitioned indexes, I think that probably the right design is one
DEPENDENCY_INTERNAL dependency on the partition master index, and
then one DEPENDENCY_AUTO dependency on the matching partitioned table.
It does not make sense to claim that an object is "part of the
implementation of" more than one thing.

If we can't do that, then ensuring stability of the report would again
require sorting of the referenced objects before we examine them.

I've not attempted to write patches for any of these ideas yet;
I thought I'd throw them out for comments first.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T21:54:36Z

    I wrote:
> I propose that we handle this case by adding a new DEPFLAG_IS_SUBOBJECT
> flag to the column object's flags, denoting that we know the whole table
> will be dropped later.  The only effect of this flag is to suppress
> reporting of the column object in reportDependentObjects.

Here's a proposed patch for that bit.  As expected, it seems to eliminate
the variation in number-of-cascaded-drops-reported under
ignore_system_indexes.  I do not see any regression outputs change
otherwise.

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-17T22:15:54Z

    On 2019-Jan-17, Tom Lane wrote:

> DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
> has no hesitation about making multiple entries of that kind.   After
> rather cursorily looking at that code, I'm leaning to the position
> that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
> nuked from orbit.  In the cases where it's being used, such as
> partitioned indexes, I think that probably the right design is one
> DEPENDENCY_INTERNAL dependency on the partition master index, and
> then one DEPENDENCY_AUTO dependency on the matching partitioned table.

As I recall, the problem with that approach is that you can't drop the
partition when a partitioned index exists, because it follows the link
to the parent index and tries to drop that.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T22:25:27Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-17, Tom Lane wrote:
>> DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
>> has no hesitation about making multiple entries of that kind.   After
>> rather cursorily looking at that code, I'm leaning to the position
>> that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
>> nuked from orbit.  In the cases where it's being used, such as
>> partitioned indexes, I think that probably the right design is one
>> DEPENDENCY_INTERNAL dependency on the partition master index, and
>> then one DEPENDENCY_AUTO dependency on the matching partitioned table.

> As I recall, the problem with that approach is that you can't drop the
> partition when a partitioned index exists, because it follows the link
> to the parent index and tries to drop that.

Hm.  Still, I can't believe that it's appropriate for a partitioned index
to have exactly the same kind of dependency on the master index as it
does on the associated table.

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-17T22:47:23Z

    On 2019-Jan-17, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Jan-17, Tom Lane wrote:
> >> DEPENDENCY_INTERNAL_AUTO, however, broke this completely, as the code
> >> has no hesitation about making multiple entries of that kind.   After
> >> rather cursorily looking at that code, I'm leaning to the position
> >> that DEPENDENCY_INTERNAL_AUTO is broken-by-design and needs to be
> >> nuked from orbit.  In the cases where it's being used, such as
> >> partitioned indexes, I think that probably the right design is one
> >> DEPENDENCY_INTERNAL dependency on the partition master index, and
> >> then one DEPENDENCY_AUTO dependency on the matching partitioned table.
> 
> > As I recall, the problem with that approach is that you can't drop the
> > partition when a partitioned index exists, because it follows the link
> > to the parent index and tries to drop that.
> 
> Hm.  Still, I can't believe that it's appropriate for a partitioned index
> to have exactly the same kind of dependency on the master index as it
> does on the associated table.

So there are three necessary features:
* The partition can be dropped, which takes down the index partition
* The master index can be dropped, which takes down the index partition
* The index partition must never be allowed to be dropped on its own.

There is a symmetry to these that led me to have the same kind of
dependency from the index partition to the other two.

I'm now wondering if it would work to have the one from index partition
to table partition as DEPENDENCY_AUTO and the one from index partition
to parent index as DEPENDENCY_INTERNAL_AUTO.  I stared at a lot of
possible dependency graphs, but I'm not sure if this one was one of
them.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T23:20:51Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-17, Tom Lane wrote:
>> Hm.  Still, I can't believe that it's appropriate for a partitioned index
>> to have exactly the same kind of dependency on the master index as it
>> does on the associated table.

> So there are three necessary features:
> * The partition can be dropped, which takes down the index partition
> * The master index can be dropped, which takes down the index partition
> * The index partition must never be allowed to be dropped on its own.

> There is a symmetry to these that led me to have the same kind of
> dependency from the index partition to the other two.

It's symmetric as long as you suppose that the above are the only
requirements.  However, there's another requirement, which is that
if you do try to drop the index partition directly, we would like
the error message to suggest dropping the master index, not the
table.  The only way to be sure about what will be suggested is
if there can be only one "owning object".

Also, I am suspicious that this implementation fails on point 3
anyway.  It looks to me like if a recursive drop reaches the
index partition from a dependency other than either the table
partition or the master index, it will let the index partition
be dropped by itself.  Ordinarily, this would likely not matter
because the index partition would share any other dependencies
(opclasses, for instance) with one or the other owning object.
But I don't think it's too hard to construct cases where that's
not true.  If nothing else, it looks like ALTER OBJECT DEPENDS ON
EXTENSION can be used to attach a random dependency to just
about anything.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-17T23:38:09Z

    On Thu, Jan 17, 2019 at 3:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > There is a symmetry to these that led me to have the same kind of
> > dependency from the index partition to the other two.
>
> It's symmetric as long as you suppose that the above are the only
> requirements.  However, there's another requirement, which is that
> if you do try to drop the index partition directly, we would like
> the error message to suggest dropping the master index, not the
> table.  The only way to be sure about what will be suggested is
> if there can be only one "owning object".

+1. This is certainly a necessary requirement. Absurd error messages
are not okay.

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T23:57:36Z

    I wrote:
> Also, I am suspicious that this implementation fails on point 3
> anyway ... If nothing else, it looks like ALTER OBJECT DEPENDS ON
> EXTENSION can be used to attach a random dependency to just
> about anything.

Yup:

regression=# drop table if exists idxpart cascade;
DROP TABLE
regression=# create table idxpart (a int) partition by range (a);
CREATE TABLE
regression=# create index on idxpart (a);
CREATE INDEX
regression=# create table idxpart1 partition of idxpart for values from (0) to (10);
CREATE TABLE
regression=# drop index idxpart1_a_idx;      -- no way
ERROR:  cannot drop index idxpart1_a_idx because index idxpart_a_idx requires it
HINT:  You can drop index idxpart_a_idx instead.
regression=# \d idxpart1
              Table "public.idxpart1"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 a      | integer |           |          | 
Partition of: idxpart FOR VALUES FROM (0) TO (10)
Indexes:
    "idxpart1_a_idx" btree (a)
regression=# create extension cube;
CREATE EXTENSION
regression=# alter index idxpart1_a_idx depends on extension cube;
ALTER INDEX
regression=# drop extension cube;
DROP EXTENSION
regression=# \d idxpart1
              Table "public.idxpart1"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 a      | integer |           |          | 
Partition of: idxpart FOR VALUES FROM (0) TO (10)


			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-18T00:03:27Z

    On Thu, Jan 17, 2019 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I poked around for awhile with running the regression tests under
> ignore_system_indexes.  There seem to be a number of issues involved
> here.  To a significant extent, they aren't bugs, at least not according
> to the original conception of the dependency code: it was not a design
> goal that different dependencies of the same object-to-be-deleted would
> be processed in a fixed order.

I agree that it's the exceptional cases that are of concern here. The
vast majority of the changes you'll see with
"ignore_system_indexes=on" are noise.

> Now, perhaps we should make such stability a design goal, as it'd allow
> us to get rid of some "suppress the cascade outputs" hacks in the
> regression tests.  But it's a bit of a new feature.  If we wanted to
> do that, I'd be inclined to do it by absorbing all the pg_depend entries
> for a particular object into an ObjectAddress array and then sorting
> them before we process them.  The main stumbling block here is "what
> would the sort order be?".  The best idea I can come up with offhand
> is to sort by OID, which at least for regression test purposes would
> mean objects would be listed/processed more or less in creation order.

I think that we might as well have a stable order. Maybe an explicit
sort step is unnecessary -- we can actually rely on scan order, while
accepting you'll get a different order with "ignore_system_indexes=on"
(though without getting substantively different/incorrect messages).
I'm slightly concerned that an explicit sort step might present
difficulties in extreme cases. How much memory are we prepared to
allocate, just to get a stable order?

It probably won't really matter what the specific order is, once the
current problems (the DEPENDENCY_INTERNAL_AUTO issue and the issue
you'll fix with DEPFLAG_IS_SUBOBJECT) are handled in a direct manner.
As I've pointed out a couple of times already, we can add a 4 byte
tie-breaker column to both pg_depend indexes without increasing the
size of the on-disk representation, since the extra space is already
lost to alignment (we could even add a new 4 byte column to the table
without any storage overhead, if that happened to make sense).

What is the likelihood that somebody will ever find a better use for
this alignment padding? These two indexes are typically the largest
system catalog indexes by far, so the opportunity cost matters. I
don't think that the direct cost (more cycles) is worth worrying
about, though. Nobody has added a pg_depend column since it was first
introduced back in 2002.

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T00:40:26Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, Jan 17, 2019 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now, perhaps we should make such stability a design goal, as it'd allow
>> us to get rid of some "suppress the cascade outputs" hacks in the
>> regression tests.  But it's a bit of a new feature.  If we wanted to
>> do that, I'd be inclined to do it by absorbing all the pg_depend entries
>> for a particular object into an ObjectAddress array and then sorting
>> them before we process them.  The main stumbling block here is "what
>> would the sort order be?".  The best idea I can come up with offhand
>> is to sort by OID, which at least for regression test purposes would
>> mean objects would be listed/processed more or less in creation order.

> I think that we might as well have a stable order. Maybe an explicit
> sort step is unnecessary -- we can actually rely on scan order, while
> accepting you'll get a different order with "ignore_system_indexes=on"
> (though without getting substantively different/incorrect messages).

Yeah, that's the policy we've followed so far, but I remain concerned
about its effects on the regression tests.  There are a lot of places
where we print full DROP CASCADE output because "it hasn't failed yet".
I fear every one of those is a gotcha that's waiting to bite us.

Also, is index scan order really guaranteed for equal-keyed items?
It isn't today, and I didn't think your patches were going to make
it so.

> I'm slightly concerned that an explicit sort step might present
> difficulties in extreme cases. How much memory are we prepared to
> allocate, just to get a stable order?

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.

> As I've pointed out a couple of times already, we can add a 4 byte
> tie-breaker column to both pg_depend indexes without increasing the
> size of the on-disk representation, since the extra space is already
> lost to alignment (we could even add a new 4 byte column to the table
> without any storage overhead, if that happened to make sense).

Meh ... where do you get the 4-byte value from?

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-18T01:09:20Z

    On Thu, Jan 17, 2019 at 4:40 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, that's the policy we've followed so far, but I remain concerned
> about its effects on the regression tests.  There are a lot of places
> where we print full DROP CASCADE output because "it hasn't failed yet".
> I fear every one of those is a gotcha that's waiting to bite us.

Right. I don't want to have to play whack-a-mole with this later on,
and risk suppressing the wrong thing.

> Also, is index scan order really guaranteed for equal-keyed items?
> It isn't today, and I didn't think your patches were going to make
> it so.

The idea is that you'd have an extra column, so they wouldn't be
equal-keyed (the keys that the scan was interested in would be
duplicated, but we could still rely on the ordering). Are you
suggesting that there'd be a stability issue regardless?

My patch does guarantee order for equal-keyed items, since heap TID is
treated as just another attribute, at least as far as the nbtree key
space is concerned. The big unknown is how exactly that works out when
the regression tests are run in parallel, on a busy system, since heap
TID isn't just another column outside of nbtree. I think that this
will probably cause test flappiness if we don't nail it down now. That
was certainly my experience before I nailed it down in my own
tentative way. My sense is that without addressing this issue, we're
going to be sensitive to concurrent heap TID recycling by VACUUM in a
way that might become an annoyance. I see no reason to not go fix it
once and for all, since the vast majority of all problems are around
the two pg_depend indexes.

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

> > As I've pointed out a couple of times already, we can add a 4 byte
> > tie-breaker column to both pg_depend indexes without increasing the
> > size of the on-disk representation, since the extra space is already
> > lost to alignment (we could even add a new 4 byte column to the table
> > without any storage overhead, if that happened to make sense).
>
> Meh ... where do you get the 4-byte value from?

In the kludgey patch that I posted, the 4-byte value is manufactured
artificially within a backend in descending order. That may have a
slight advantage over object oid, even after the pg_depend correctness
issues are addressed. A fixed order within a backend or originating
transaction seems like it might avoid a few remaining instability
issues. Not sure. I seem to recall there being some disagreement
between you and Andres on this very point (is object oid a perfectly
stable tie-breaker in practice?) on a similar thread from 2017.

-- 
Peter Geoghegan

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-18T01:15:23Z

    On Thu, Jan 17, 2019 at 5:09 PM Peter Geoghegan <pg@bowt.ie> wrote:
> In the kludgey patch that I posted, the 4-byte value is manufactured
> artificially within a backend in descending order. That may have a
> slight advantage over object oid, even after the pg_depend correctness
> issues are addressed. A fixed order within a backend or originating
> transaction seems like it might avoid a few remaining instability
> issues. Not sure. I seem to recall there being some disagreement
> between you and Andres on this very point (is object oid a perfectly
> stable tie-breaker in practice?) on a similar thread from 2017.

Here are your remarks about it on that 2017 thread:
https://www.postgresql.org/message-id/11852.1501610262%40sss.pgh.pa.us

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T23:34:00Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Thu, Jan 17, 2019 at 4:40 PM Tom Lane <tgl@sss.pgh.pa.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

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-19T00:06:58Z

    On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

This reminds me of the output that I saw back when my patch used DESC
heap TID order. I agree that those regression test changes are
improvements. I think that they're caused by the existing nbtree
code's preference for storing duplicates on the first leaf page it
could go on that is found to be empty. It stores duplicates in
approximately descending order, but now you're artificially forcing
perfectly descending order -- one or two things that were not in the
expected (reverse) chronological order, by accident, now appear in
order.

(I've not verified this explanation, though.)

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

I doubt it myself. We can always change it later on.

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

The nbtree patch is not dependent on doing better here, since the
order here is merely unstable, without leading to incorrect diagnostic
messages. I can just change the regress output mechanically. That
said, I cannot be sure that the instability won't become more of an
annoyance with heap TID being treated as a tie-breaker attribute
within nbtree. It's probably fine to reserve the option to do better
here, and do so if and when it becomes clear that it matters. I defer
to you.

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

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?

Thanks
-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-19T00:24:18Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

> This reminds me of the output that I saw back when my patch used DESC
> heap TID order. I agree that those regression test changes are
> improvements. I think that they're caused by the existing nbtree
> code's preference for storing duplicates on the first leaf page it
> could go on that is found to be empty.

Yeah, I figured the explanation for the weirder changes was somewhere
around there.  Like you, I haven't bothered to verify 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.

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

It's clear that we must change *something* in that area.  I'm not yet
wedded to a particular fix, just expressing a guess as to what might
be the cleanest fix.

Also, we evidently need something we can back-patch into v11, which might
end up being very far from clean :-(.  I have no opinions yet on what
would make sense in that branch.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-19T01:06:26Z

    On Fri, Jan 18, 2019 at 4:24 PM Tom Lane <tgl@sss.pgh.pa.us> 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?
>
> It's clear that we must change *something* in that area.  I'm not yet
> wedded to a particular fix, just expressing a guess as to what might
> be the cleanest fix.

I'm surprised that you're not "wedded" to that fix in some sense,
though. Your analysis about the right design having one
DEPENDENCY_INTERNAL dependency on the partition master index, and one
DEPENDENCY_AUTO dependency on the matching partitioned table seemed
spot on to me.

> Also, we evidently need something we can back-patch into v11, which might
> end up being very far from clean :-(.  I have no opinions yet on what
> would make sense in that branch.

Me neither, but, as I said, I think that you've identified the right
design for the master branch. And, I tend to doubt that you'll find
something that works for the backbranches that is also worth using in
the master branch.

Why does it seem necessary to fix the bug in the backbranches? I agree
that it's broken, but it's not obvious to me that it'll cause serious
problems in the real world that outweigh the potential downsides.
Perhaps I've missed some obvious downside.


--
Peter Geoghegan

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-19T02:01:09Z

    On 2019-Jan-18, Peter Geoghegan wrote:

> On Fri, Jan 18, 2019 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Peter Geoghegan <pg@bowt.ie> — 2019-01-21T21:19:54Z

    On Fri, Jan 18, 2019 at 4:06 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > * 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.
>
> The nbtree patch is not dependent on doing better here, since the
> order here is merely unstable, without leading to incorrect diagnostic
> messages. I can just change the regress output mechanically. That
> said, I cannot be sure that the instability won't become more of an
> annoyance with heap TID being treated as a tie-breaker attribute
> within nbtree. It's probably fine to reserve the option to do better
> here, and do so if and when it becomes clear that it matters. I defer
> to you.

Good news: Testing my patch (with the hacky pg_depend tie-breaker
commit removed) on top of your recent commit f1ad067f confirms the
only "real" remaining problem is with two tests that relate to
partitioning (the multiple DEPENDENCY_INTERNAL_AUTO entry issue). It
seems likely that objects-to-drop output from DROP ROLE can remain
"unstable" without bothering anybody -- I've run "make -Otarget -j10
-s check-world" with the same source tree multiple times, and have yet
to see a test failure. In practice *all* of the instability that's of
practical concern (that could cause buildfarm failures) related to the
two pg_depend indexes. It very much looks that way, at least -- only
the buildfarm can confirm this.

I attach a patch that shows how I'll adjust the harmless
objects-to-drop output from DROP ROLE to make relevant tests pass --
the scope of changes is very limited, and the changes are
harmless/mechanical. Separately, I attach the patch that I'd have to
use to paper over the partitioning/DEPENDENCY_INTERNAL_AUTO issue (a
patch that makes the regression tests actively ignore visible
manifestations of the DEPENDENCY_INTERNAL_AUTO bug). As I said, this
confirms that there are only two "real" remaining tests that fail.

Thanks
-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-29T19:28:44Z

    Alvaro Herrera <alvherre@2ndquadrant.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

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-05T21:28:29Z

    On 2019-Jan-29, Tom Lane wrote:

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

I agree it's pretty weird :-(

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

I don't put a lot of value in the idea of people going back in minor
releases, but I agree with the general principle of not making pg_depend
any harder to inspect than it already is.  Even so, I wouldn't dismiss a
solution along these lines, if it was clean enough; we could provide
better tools for pg_depend inspection (some sensible view, at least).
It might make sense to implement this in master only, if it turns out to
be better in some way.  I'm not volunteering right now, but maybe in the
future.

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

This seems a great idea.  Do you want me to implement it?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-05T22:08:48Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jan-29, Tom Lane wrote:
>> 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.

> This seems a great idea.  Do you want me to implement it?

I've got much of the code for it already (in the wreckage of my failed
attempts), so I'll go back and finish that up.  I was just waiting to see
how loudly people would howl about using object type as a condition for
figuring out what a pg_depend entry really means.  If we're okay with
that hack, I think I can make it work.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-05T22:50:34Z

    On Tue, Feb 5, 2019 at 2:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've got much of the code for it already (in the wreckage of my failed
> attempts), so I'll go back and finish that up.  I was just waiting to see
> how loudly people would howl about using object type as a condition for
> figuring out what a pg_depend entry really means.  If we're okay with
> that hack, I think I can make it work.

Perhaps I've missed some subtlety, but I'm not sure that it's all that
ugly. If splitting INTERNAL_AUTO into two new dependency types amounts
to the same thing as what you suggest here, then what's the
difference? If this secondary INTERNAL_AUTO entry property can be
determined from the pg_depend record alone with either approach, then
it's not obvious to me that an "explicit representation" buys us
anything. Yes, you must introduce a special case...but isn't it a
special case either way?

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-06T03:21:35Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Tue, Feb 5, 2019 at 2:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I've got much of the code for it already (in the wreckage of my failed
>> attempts), so I'll go back and finish that up.  I was just waiting to see
>> how loudly people would howl about using object type as a condition for
>> figuring out what a pg_depend entry really means.  If we're okay with
>> that hack, I think I can make it work.

> Perhaps I've missed some subtlety, but I'm not sure that it's all that
> ugly. If splitting INTERNAL_AUTO into two new dependency types amounts
> to the same thing as what you suggest here, then what's the
> difference?

It's the same as long as we always think that the "real owner" of a
subsidiary object is of the same type as that object, eg that the
real owner of a per-partition trigger is the parent trigger, the real
owner of a per-partition index is the parent index, etc ... and that
there's only going to be one parent object of that type.

I don't immediately have a counterexample to this, which is why I feel
like this is an OK solution for now.  But I'm not sure it'll hold good
indefinitely.

Actually, the index case is already on the edge of being a problem:
both parents will be relations, and I suspect the code will have to
look at relkinds to identify which parent to consider the "real owner".

BTW, does anyone else feel like our terminology around partitions is
a dead loss?  I have not seen anybody use the word in a way that makes
a clear distinction between the parent "partitioned" object and the
child "partition" objects, at least not when it comes to subsidiary
objects like triggers.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-07T05:14:58Z

    I wrote:
>>> I've got much of the code for it already (in the wreckage of my failed
>>> attempts), so I'll go back and finish that up.

So I've been poking at that for most of the day, and I've despaired of
being able to fix this in v11.  The problem is that somebody was rolling
dice while deciding which dependencies of what types to insert for
partitioning cases.  In particular, try this (extracted from
foreign_key.sql):

CREATE TABLE fk_notpartitioned_pk (a int, b int, PRIMARY KEY (a, b));

CREATE TABLE fk_partitioned_fk (b int, a int) PARTITION BY RANGE (a, b);
ALTER TABLE fk_partitioned_fk ADD FOREIGN KEY (a, b) REFERENCES fk_notpartitioned_pk;

CREATE TABLE fk_partitioned_fk_2 PARTITION OF fk_partitioned_fk FOR VALUES FROM (1,1) TO (10,10);

At this point the dependencies of the child table's FK constraint look
like

select
  pg_describe_object(classid, objid, objsubid) as obj,
  pg_describe_object(refclassid, refobjid, refobjsubid) as ref,
  deptype, objid, refobjid
from pg_depend where objid =
  (select oid from pg_constraint where conrelid = 'fk_partitioned_fk_2'::regclass);

                               obj                                |                              ref                               | deptype | objid | refobjid 
------------------------------------------------------------------+----------------------------------------------------------------+---------+-------+----------
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column a of table fk_partitioned_fk_2                          | a       | 52787 |    52784
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column b of table fk_partitioned_fk_2                          | a       | 52787 |    52784
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column a of table fk_notpartitioned_pk                         | n       | 52787 |    37209
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | column b of table fk_notpartitioned_pk                         | n       | 52787 |    37209
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | index fk_notpartitioned_pk_pkey                                | n       | 52787 |    37215
 constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk_2 | constraint fk_partitioned_fk_a_fkey on table fk_partitioned_fk | I       | 52787 |    52781
(6 rows)

Now, v11 will let you drop the fk_partitioned_fk_2 table, which
recurses to dropping this constraint, and it does not then complain
that you should've dropped the fk_partitioned_fk_a_fkey constraint.
Given these dependencies, that's just *wrong*.  This dependency set
is no different from the bug case I exhibited upthread: arriving at
this object via an auto dependency should not be enough to let it
be dropped, if there's an 'I' dependency for it.

It's also fair to wonder what we're doing applying a single 'I'
dependency to an object in the first place; why not use a regular
internal dependency in that case?

Another problem I came across can be exhibited like this:

CREATE TABLE prt1 (a int, b int, c int) PARTITION BY RANGE(a);
CREATE INDEX ON prt1(c);

CREATE TABLE prt1_p3 PARTITION OF prt1 FOR VALUES FROM (500) TO (600);

alter table prt1 DETACH PARTITION prt1_p3;

At this point we have

select
  pg_describe_object(classid, objid, objsubid) as obj,
  pg_describe_object(refclassid, refobjid, refobjsubid) as ref,
  deptype, objid, refobjid
from pg_depend where 'prt1_p3_c_idx'::regclass in (objid,refobjid);

         obj         |      ref      | deptype | objid | refobjid 
---------------------+---------------+---------+-------+----------
 index prt1_p3_c_idx | table prt1_p3 | a       | 52797 |    52794
(1 row)

This is also flat wrong, because it wil let you do

alter table prt1_p3 drop column c;

and the index still exists, though in a broken state:

\d prt1_p3

              Table "public.prt1_p3"
 Column |  Type   | Collation | Nullable | Default 
--------+---------+-----------+----------+---------
 a      | integer |           |          | 
 b      | integer |           |          | 
Indexes:
    "prt1_p3_c_idx" btree ("........pg.dropped.3........" int4_ops)

The reason for that is that IndexSetParentIndex thinks that if it's
unlinking an index from a partition index, all it needs to put back
is an auto dependency on the whole table.  This has exactly nothing
to do with the dependencies that the index would normally have;
those are usually column-level not table-level.  As this example shows,
you do not get to take shortcuts.

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.

I have a mostly-working patch along these lines that I hope to
finish up and post tomorrow.  But there's no hope of applying it
to v11 --- if presented with the dependencies that v11 is currently
storing, the patch would refuse deletion of partitioned tables altogether
in many common cases.  That cure's certainly worse than the disease.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T02:53:34Z

    On Wed, Feb 6, 2019 at 9:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I have a mostly-working patch along these lines that I hope to
> finish up and post tomorrow.

Do you think that you'll end up pushing the HEAD-only fix shortly? It
would be nice to avoid immediate bitrot of an upcoming revision of the
nbtree patch series.

Thanks
-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-08T03:35:34Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Wed, Feb 6, 2019 at 9:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I have a mostly-working patch along these lines that I hope to
>> finish up and post tomorrow.

> Do you think that you'll end up pushing the HEAD-only fix shortly?

I have a working patch now, but I think I'm too tired to explain it,
so I'm going to post it tomorrow instead.  It's a big enough change
that it might be advisable for someone to review it --- are you
interested?

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T03:43:15Z

    On Thu, Feb 7, 2019 at 7:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I have a working patch now, but I think I'm too tired to explain it,
> so I'm going to post it tomorrow instead.  It's a big enough change
> that it might be advisable for someone to review it --- are you
> interested?

I'd be happy to review it.

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-08T16:15:39Z

    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

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-08T20:19:25Z

    On 2019-Feb-08, Tom Lane wrote:

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

Oops, this is new code in 0464fdf07f69 (Jan 21st).  Unless you object,
I'll study a fix for this now, to avoid letting it appear in the minor
next week.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-08T21:40:14Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T00:41:40Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-08, Tom Lane wrote:
>> 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.

> Oops, this is new code in 0464fdf07f69 (Jan 21st).  Unless you object,
> I'll study a fix for this now, to avoid letting it appear in the minor
> next week.

+1.  The best solution would presumably be to go through the normal
object deletion mechanism; though possibly there's a reason that
won't work given you're already inside some other DDL.

			regards, tom lane

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

    Amit Langote <amitlangote09@gmail.com> — 2019-02-09T07:14:41Z

    On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-08, Tom Lane wrote:
> >> 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.
>
> > Oops, this is new code in 0464fdf07f69 (Jan 21st).  Unless you object,
> > I'll study a fix for this now, to avoid letting it appear in the minor
> > next week.
>
> +1.  The best solution would presumably be to go through the normal
> object deletion mechanism; though possibly there's a reason that
> won't work given you're already inside some other DDL.

Maybe:

- CatalogTupleDelete(trigrel, &trigtup->t_self);
+ RemoveTriggerById(trgform->oid)?

Thanks,
Amit

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T07:26:00Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> +1.  The best solution would presumably be to go through the normal
>> object deletion mechanism; though possibly there's a reason that
>> won't work given you're already inside some other DDL.

> Maybe:
> - CatalogTupleDelete(trigrel, &trigtup->t_self);
> + RemoveTriggerById(trgform->oid)?

No, that's still the back end of the deletion machinery, and in particular
it would fail to clean pg_depend entries for the trigger.  Going in by the
front door would use performDeletion().  (See deleteOneObject() to get
an idea of what's being possibly missed out here.)

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-09T15:56:13Z

    On 2019-Feb-09, Tom Lane wrote:

> Amit Langote <amitlangote09@gmail.com> writes:
> > On Sat, Feb 9, 2019 at 9:41 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> +1.  The best solution would presumably be to go through the normal
> >> object deletion mechanism; though possibly there's a reason that
> >> won't work given you're already inside some other DDL.
> 
> > Maybe:
> > - CatalogTupleDelete(trigrel, &trigtup->t_self);
> > + RemoveTriggerById(trgform->oid)?
> 
> No, that's still the back end of the deletion machinery, and in particular
> it would fail to clean pg_depend entries for the trigger.  Going in by the
> front door would use performDeletion().  (See deleteOneObject() to get
> an idea of what's being possibly missed out here.)

This patch I think does the right thing.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T16:10:38Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> No, that's still the back end of the deletion machinery, and in particular
>> it would fail to clean pg_depend entries for the trigger.  Going in by the
>> front door would use performDeletion().  (See deleteOneObject() to get
>> an idea of what's being possibly missed out here.)

> This patch I think does the right thing.

(squint ...) Don't much like the undocumented deleteDependencyRecordsFor
call; that looks like it's redundant with what deleteOneObject will do.
I think you're doing it to get rid of the INTERNAL dependency so that
deletion won't recurse across that, but why is that a good idea?  Needs
a comment at least.

Also, I suspect you might need a second CCI after the performDeletion
call, in case the loop iterates?

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-09T16:50:10Z

    On 2019-Feb-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> No, that's still the back end of the deletion machinery, and in particular
> >> it would fail to clean pg_depend entries for the trigger.  Going in by the
> >> front door would use performDeletion().  (See deleteOneObject() to get
> >> an idea of what's being possibly missed out here.)
> 
> > This patch I think does the right thing.
> 
> (squint ...) Don't much like the undocumented deleteDependencyRecordsFor
> call; that looks like it's redundant with what deleteOneObject will do.
> I think you're doing it to get rid of the INTERNAL dependency so that
> deletion won't recurse across that, but why is that a good idea?  Needs
> a comment at least.

Yeah, it's deleting the INTERNAL dependency, because otherwise the
trigger deletion is (correctly) forbidden, since the constraint depends
on it.  Perhaps it'd be good to have it be more targetted: make sure it
only deletes that dependency row and not any others that the trigger
might have (though I don't have it shouldn't have any.  How could it?)  I'd do
that by adding a new function

long
deleteExactDependencyRecords(Oid classId, Oid objectId,
                             Oid refclassId, Oid refobjectId)

and calling that instead of deleteDependencyRecordsFor.

> Also, I suspect you might need a second CCI after the performDeletion
> call, in case the loop iterates?

Can do.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Amit Langote <amitlangote09@gmail.com> — 2019-02-09T17:06:47Z

    On Sun, Feb 10, 2019 at 1:50 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2019-Feb-09, Tom Lane wrote:
> > Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > > On 2019-Feb-09, Tom Lane wrote:
> > >> No, that's still the back end of the deletion machinery, and in particular
> > >> it would fail to clean pg_depend entries for the trigger.  Going in by the
> > >> front door would use performDeletion().  (See deleteOneObject() to get
> > >> an idea of what's being possibly missed out here.)
> >
> > > This patch I think does the right thing.
> >
> > (squint ...) Don't much like the undocumented deleteDependencyRecordsFor
> > call; that looks like it's redundant with what deleteOneObject will do.
> > I think you're doing it to get rid of the INTERNAL dependency so that
> > deletion won't recurse across that, but why is that a good idea?  Needs
> > a comment at least.
>
> Yeah, it's deleting the INTERNAL dependency, because otherwise the
> trigger deletion is (correctly) forbidden, since the constraint depends
> on it.  Perhaps it'd be good to have it be more targetted: make sure it
> only deletes that dependency row and not any others that the trigger
> might have (though I don't have it shouldn't have any.  How could it?)

Reading Tom's reply to my email, I wondered if performDeletion won't
do more than what the code is already doing (except calling the right
trigger deletion function which the current code doesn't), because the
trigger in question is an internal trigger without any dependencies
(the function it invokes are pinned by the system)?

Thanks,
Amit

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T17:08:20Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> I think you're doing it to get rid of the INTERNAL dependency so that
>> deletion won't recurse across that, but why is that a good idea?  Needs
>> a comment at least.

> Yeah, it's deleting the INTERNAL dependency, because otherwise the
> trigger deletion is (correctly) forbidden, since the constraint depends
> on it.

Well, the question that's begged here is exactly why it's okay to remove
the trigger and dependency link despite the fact that the constraint needs
it.  I suppose the answer is that we'll subsequently insert a new trigger
implementing the same constraint (and internally-linked to it)?  That
information is what I'd like to have in the comment.

> Perhaps it'd be good to have it be more targetted: make sure it
> only deletes that dependency row and not any others that the trigger
> might have (though I don't have it shouldn't have any.  How could it?)  I'd do
> that by adding a new function

I'm not sure that'd be an improvement, especially in light of the
hazard that the trigger might somehow have acquired extension and/or
partition dependencies that'd also cause issues.

			regards, tom lane

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T17:13:01Z

    Amit Langote <amitlangote09@gmail.com> writes:
> Reading Tom's reply to my email, I wondered if performDeletion won't
> do more than what the code is already doing (except calling the right
> trigger deletion function which the current code doesn't), because the
> trigger in question is an internal trigger without any dependencies
> (the function it invokes are pinned by the system)?

A big part of the point here is to not have to have such assumptions
wired into the fk-cloning code.  But even if that internal dependency is
the only one the trigger is involved in, there are other steps in
deleteOneObject that shouldn't be ignored.  For example, somebody
could've attached a comment to it.

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-09T17:41:23Z

    On 2019-Feb-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> I think you're doing it to get rid of the INTERNAL dependency so that
> >> deletion won't recurse across that, but why is that a good idea?  Needs
> >> a comment at least.
> 
> > Yeah, it's deleting the INTERNAL dependency, because otherwise the
> > trigger deletion is (correctly) forbidden, since the constraint depends
> > on it.
> 
> Well, the question that's begged here is exactly why it's okay to remove
> the trigger and dependency link despite the fact that the constraint needs
> it.  I suppose the answer is that we'll subsequently insert a new trigger
> implementing the same constraint (and internally-linked to it)?  That
> information is what I'd like to have in the comment.

Well, the answer is that the trigger is no longer needed.  This is an
action trigger, i.e. it's attached to the referenced relation; and the
action is making an independent table become a partition.  Since the
partition is reachable by the action trigger that goes through the
parent table, we no longer need the action trigger that goes directly to
the partition.

Conversely, when we detach a partition, we create an action trigger that
wasn't there before.

I'll put this in the comment.

> > Perhaps it'd be good to have it be more targetted: make sure it
> > only deletes that dependency row and not any others that the trigger
> > might have (though I don't have it shouldn't have any.  How could it?)  I'd do
> > that by adding a new function
> 
> I'm not sure that'd be an improvement, especially in light of the
> hazard that the trigger might somehow have acquired extension and/or
> partition dependencies that'd also cause issues.

Good point.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Amit Langote <amitlangote09@gmail.com> — 2019-02-09T17:51:38Z

    On Sun, Feb 10, 2019 at 2:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > Reading Tom's reply to my email, I wondered if performDeletion won't
> > do more than what the code is already doing (except calling the right
> > trigger deletion function which the current code doesn't), because the
> > trigger in question is an internal trigger without any dependencies
> > (the function it invokes are pinned by the system)?
>
> A big part of the point here is to not have to have such assumptions
> wired into the fk-cloning code.  But even if that internal dependency is
> the only one the trigger is involved in, there are other steps in
> deleteOneObject that shouldn't be ignored.  For example, somebody
> could've attached a comment to it.

Okay, I hadn't considered that far.  Thanks for explaining.

Regards,
Amit

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-09T18:56:46Z

    On 2019-Feb-09, Alvaro Herrera wrote:

> I'll put this in the comment.

Attached.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-09T20:26:55Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Well, the question that's begged here is exactly why it's okay to remove
>> the trigger and dependency link despite the fact that the constraint needs
>> it.  I suppose the answer is that we'll subsequently insert a new trigger
>> implementing the same constraint (and internally-linked to it)?  That
>> information is what I'd like to have in the comment.

> Well, the answer is that the trigger is no longer needed.  This is an
> action trigger, i.e. it's attached to the referenced relation; and the
> action is making an independent table become a partition.  Since the
> partition is reachable by the action trigger that goes through the
> parent table, we no longer need the action trigger that goes directly to
> the partition.

Oh ... then why don't we go ahead and get rid of the constraint entry,
too?

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-10T01:16:54Z

    On 2019-Feb-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> Well, the question that's begged here is exactly why it's okay to
> >> remove the trigger and dependency link despite the fact that the
> >> constraint needs it.  I suppose the answer is that we'll
> >> subsequently insert a new trigger implementing the same constraint
> >> (and internally-linked to it)?  That information is what I'd like
> >> to have in the comment.
> 
> > Well, the answer is that the trigger is no longer needed.  This is
> > an action trigger, i.e. it's attached to the referenced relation;
> > and the action is making an independent table become a partition.
> > Since the partition is reachable by the action trigger that goes
> > through the parent table, we no longer need the action trigger that
> > goes directly to the partition.
> 
> Oh ... then why don't we go ahead and get rid of the constraint entry,
> too?

Because each partition has its own pg_constraint entry.  (Otherwise
there's no place to put the column numbers into -- they can differ from
partition to partition, remember.)  The only thing we do is mark it as
child of the parent's one.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T01:25:37Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Oh ... then why don't we go ahead and get rid of the constraint entry,
>> too?

> Because each partition has its own pg_constraint entry.  (Otherwise
> there's no place to put the column numbers into -- they can differ from
> partition to partition, remember.)  The only thing we do is mark it as
> child of the parent's one.

Uh-huh.  And what happens after DETACH PARTITION ... are you going to run
around and recreate these triggers?

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-10T01:26:59Z

    On 2019-Feb-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> Oh ... then why don't we go ahead and get rid of the constraint entry,
> >> too?
> 
> > Because each partition has its own pg_constraint entry.  (Otherwise
> > there's no place to put the column numbers into -- they can differ from
> > partition to partition, remember.)  The only thing we do is mark it as
> > child of the parent's one.
> 
> Uh-huh.  And what happens after DETACH PARTITION ... are you going to run
> around and recreate these triggers?

Yep, that's there too.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T01:41:06Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-09, Tom Lane wrote:
>> Uh-huh.  And what happens after DETACH PARTITION ... are you going to run
>> around and recreate these triggers?

> Yep, that's there too.

OK, then I guess it's fine.

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-10T13:05:53Z

    On 2019-Feb-09, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2019-Feb-09, Tom Lane wrote:
> >> Uh-huh.  And what happens after DETACH PARTITION ... are you going to run
> >> around and recreate these triggers?
> 
> > Yep, that's there too.
> 
> OK, then I guess it's fine.

Thanks for verifying; pushed now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T16:12:58Z

    Peter Geoghegan <pg@bowt.ie> writes:
> On Fri, Feb 8, 2019 at 8:15 AM Tom Lane <tgl@sss.pgh.pa.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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T16:50:19Z

    I wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
>>> [ invent separate primary and secondary partition dependencies? ]

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

> Me too.

Here's a version of the patch that does it that way.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-10T18:01:19Z

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

LGTM. I agree that referencing a counterfactual design that reacquires
the lock instead adds something.

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

I thought that you might have had a trick up your sleeve for v11,
although I had no idea how that would be possible without making sure
that partition dependencies came in pairs to begin with.  :-)

I'll reply to your new revision of the patch separately.

-- 
Peter Geoghegan

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-10T18:22:37Z

    On Sun, Feb 10, 2019 at 8:50 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> [ invent separate primary and secondary partition dependencies? ]

> Here's a version of the patch that does it that way.

Now that I see separate DEPENDENCY_PARTITION_PRI and
DEPENDENCY_PARTITION_SEC dependency types, I agree that it's clearer
that way. It certainly clarifies what external dependency.c callers
are up to.

Minor issue here:

> !     <varlistentry>
> !      <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term>
> !      <listitem>
> !       <para>
> !        A <quote>secondary</quote> partition dependency acts identically to
> !        a primary one, except that the primary dependency is preferentially
> !        referenced in error messages.  An object should have at most one
> !        primary partition dependency, but there could perhaps be multiple
> !        secondary dependencies.
> !        Example: actually, we'll set up a child partitioned index with the
> !        parent partitioned index as primary partition dependency and the
> !        partition table as secondary partition dependency.  In this way,
> !        if the user tries to drop the child partitioned index, the error
> !        message will suggest dropping the parent partitioned index instead
> !        (not the table).
>         </para>

I think that the wording for this example needs to be tweaked.

Other than that, looks good to me.

-- 
Peter Geoghegan

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T19:34:06Z

    Peter Geoghegan <pg@bowt.ie> writes:
> I think that the wording for this example needs to be tweaked.
> Other than that, looks good to me.

After looking closer, I find that it's valid SGML to collapse the two
items into one entry, so how about:

    <varlistentry>
     <term><symbol>DEPENDENCY_PARTITION_PRI</symbol> (<literal>P</literal>)</term>
     <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term>
     <listitem>
      <para>
       The dependent object was created as part of creation of the
       referenced object, and is really just a part of its internal
       implementation; however, unlike <literal>INTERNAL</literal>,
       there is more than one such referenced object.  The dependent object
       must not be dropped unless at least one of these referenced objects
       is dropped; if any one is, the dependent object should be dropped
       whether or not <literal>CASCADE</literal> is specified.  Also
       unlike <literal>INTERNAL</literal>, a drop of some other object
       that the dependent object depends on does not result in automatic
       deletion of any partition-referenced object.  Hence, if the drop
       does not cascade to at least one of these objects via some other
       path, it will be refused.  (In most cases, the dependent object
       shares all its non-partition dependencies with at least one
       partition-referenced object, so that this restriction does not
       result in blocking any cascaded delete.)
       Primary and secondary partition dependencies behave identically
       except that the primary dependency is preferred for use in error
       messages; hence, a partition-dependent object should have one
       primary partition dependency and one or more secondary partition
       dependencies.
       Note that partition dependencies are made in addition to, not
       instead of, any dependencies the object would normally have.  This
       simplifies <command>ATTACH/DETACH PARTITION</command> operations:
       the partition dependencies need only be added or removed.
       Example: a child partitioned index is made partition-dependent
       on both the partition table it is on and the parent partitioned
       index, so that it goes away if either of those is dropped, but
       not otherwise.  The dependency on the parent index is primary,
       so that if the user tries to drop the child partitioned index,
       the error message will suggest dropping the parent index instead
       (not the table).
      </para>
     </listitem>
    </varlistentry>

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-10T19:53:58Z

    On Sun, Feb 10, 2019 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> After looking closer, I find that it's valid SGML to collapse the two
> items into one entry

I'll have to remember that detail -- seems like it'll come in handy again.

>     <varlistentry>
>      <term><symbol>DEPENDENCY_PARTITION_PRI</symbol> (<literal>P</literal>)</term>
>      <term><symbol>DEPENDENCY_PARTITION_SEC</symbol> (<literal>S</literal>)</term>
>      <listitem>
>       <para>

>        Primary and secondary partition dependencies behave identically
>        except that the primary dependency is preferred for use in error
>        messages; hence, a partition-dependent object should have one
>        primary partition dependency and one or more secondary partition
>        dependencies.
>        Note that partition dependencies are made in addition to, not
>        instead of, any dependencies the object would normally have.  This
>        simplifies <command>ATTACH/DETACH PARTITION</command> operations:
>        the partition dependencies need only be added or removed.
>        Example: a child partitioned index is made partition-dependent
>        on both the partition table it is on and the parent partitioned
>        index, so that it goes away if either of those is dropped, but
>        not otherwise.  The dependency on the parent index is primary,
>        so that if the user tries to drop the child partitioned index,
>        the error message will suggest dropping the parent index instead
>        (not the table).

That seems perfect. It gets to the root of the matter.

-- 
Peter Geoghegan

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-10T20:47:56Z

    On 2019-Feb-10, Tom Lane wrote:

>        Primary and secondary partition dependencies behave identically
>        except that the primary dependency is preferred for use in error
>        messages; hence, a partition-dependent object should have one
>        primary partition dependency and one or more secondary partition
>        dependencies.

Hmm, zero or more secondary partition dependencies?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T20:51:36Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-10, Tom Lane wrote:
>> Primary and secondary partition dependencies behave identically
>> except that the primary dependency is preferred for use in error
>> messages; hence, a partition-dependent object should have one
>> primary partition dependency and one or more secondary partition
>> dependencies.

> Hmm, zero or more secondary partition dependencies?

If there's only one partition dependency, why use the mechanism
at all?

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-10T20:54:49Z

    On 2019-Feb-10, Peter Geoghegan wrote:

> On Sun, Feb 10, 2019 at 8:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> > 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.
> 
> I thought that you might have had a trick up your sleeve for v11,
> although I had no idea how that would be possible without making sure
> that partition dependencies came in pairs to begin with.  :-)

If we disregard the scenario were people downgrade across minor
versions, it's likely possible to produce SQL queries to transform from
the old arrangement to the new one, and include those in release notes
or a wiki page; not for this week's minors (ENOTIME) but maybe for the
next one.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-10T21:06:29Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-10, Peter Geoghegan wrote:
>> On Sun, Feb 10, 2019 at 8:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> 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.

>> I thought that you might have had a trick up your sleeve for v11,
>> although I had no idea how that would be possible without making sure
>> that partition dependencies came in pairs to begin with.  :-)

> If we disregard the scenario were people downgrade across minor
> versions, it's likely possible to produce SQL queries to transform from
> the old arrangement to the new one, and include those in release notes
> or a wiki page; not for this week's minors (ENOTIME) but maybe for the
> next one.

Dunno ... we couldn't force people to do that, so the server would have to
be prepared to cope with either arrangement, which seems like an
impossible mess.

			regards, tom lane

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-11T01:54:23Z

    On 2019-Feb-10, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:

> > If we disregard the scenario were people downgrade across minor
> > versions, it's likely possible to produce SQL queries to transform from
> > the old arrangement to the new one, and include those in release notes
> > or a wiki page; not for this week's minors (ENOTIME) but maybe for the
> > next one.
> 
> Dunno ... we couldn't force people to do that, so the server would have to
> be prepared to cope with either arrangement, which seems like an
> impossible mess.

True.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-11T19:46:43Z

    I've pushed this now.  I made one additional change, which was to fix
things so that if both an INTERNAL and an EXTENSION dependency exist,
the first loop will reliably complain about the EXTENSION dependency.
It only takes one more if-test to do that now that we're postponing
the error report till after the loop, and this way we don't need to
split hairs about how likely it is for both to exist.

I think we're done with this thread, though I still need to look at
the problem I complained of in <26527.1549572789@sss.pgh.pa.us>.

			regards, tom lane

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

    Peter Geoghegan <pg@bowt.ie> — 2019-02-11T19:54:13Z

    On Mon, Feb 11, 2019 at 11:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think we're done with this thread, though I still need to look at
> the problem I complained of in <26527.1549572789@sss.pgh.pa.us>.

Right, we're done with this thread now. Thanks again!

-- 
Peter Geoghegan

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

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-11T20:04:45Z

    On 2019-Feb-11, Tom Lane wrote:

> I've pushed this now.  I made one additional change, which was to fix
> things so that if both an INTERNAL and an EXTENSION dependency exist,
> the first loop will reliably complain about the EXTENSION dependency.
> It only takes one more if-test to do that now that we're postponing
> the error report till after the loop, and this way we don't need to
> split hairs about how likely it is for both to exist.
> 
> I think we're done with this thread, though I still need to look at
> the problem I complained of in <26527.1549572789@sss.pgh.pa.us>.

Thanks for taking care of this!

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services