Subject: Re: Fixing findDependentObjects()'s dependency on scan order
 (regressions in DROP diagnostic messages)
To: Peter Geoghegan <pg@bowt.ie>, Alvaro Herrera <alvherre@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: 
 <CAH2-Wzkypv1R+teZrr71U23J578NnTBt2X8+Y=Odr4pOdW1rXg@mail.gmail.com>
From: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
Message-ID: <0a89eb40-ea89-9037-aad0-3b65c67cdcbd@postgrespro.ru>
Date: Tue, 6 Nov 2018 08:46:17 +0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: 
 <CAH2-Wzkypv1R+teZrr71U23J578NnTBt2X8+Y=Odr4pOdW1rXg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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