Subject: Re: \d with triggers: more than one row returned by a subquery used as an expression

From: Justin Pryzby <[email-masked]>
Date: Mon, 17 Jan 2022 18:08:24 -0600

On Mon, Jan 17, 2022 at 05:02:00PM -0500, Tom Lane wrote:
> Justin Pryzby <[email-masked]> writes:
> > On Fri, Dec 17, 2021 at 09:43:56AM -0600, Justin Pryzby wrote:
> >> I want to mention that the 2nd problem I mentioned here is still broken.
> >> https://www.postgresql.org/message-id/[email-masked]
> >> It happens if non-inheritted triggers on child and parent have the same name.
> 
> > This is the fix I was proposing
> > It depends on pg_partition_ancestors() to return its partitions in order:
> > this partition => parent => ... => root.
> 
> I don't think that works at all.  I might be willing to accept the
> assumption about pg_partition_ancestors()'s behavior, but you're also
> making an assumption about how the output of pg_partition_ancestors()
> is joined to "pg_trigger AS u", and I really don't think that's safe.

> ISTM the real problem is the assumption that only related triggers could
> share a tgname, which evidently isn't true.  I think this query needs to
> actually match on tgparentid, rather than taking shortcuts.

I don't think that should be needed - tgparentid should match
pg_partition_ancestors().

> If we don't
> want to use a recursive CTE, maybe we could define it as only looking up to
> the immediate parent, rather than necessarily finding the root?

I think that defeats the intent of c33869cc3.

Is there any reason why WITH ORDINALITY can't work ?
This is passing the smoke test.

-- 
Justin
>From 99f8ee921258d9b2e75299e69826004fda3b8919 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH 1/2] psql: Add newlines and spacing in trigger query for \d

cosmetic change to c33869cc3 to make the output of psql -E look pretty.
---
 src/bin/psql/describe.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 61cec118aca..ed9f320b015 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2993,12 +2993,12 @@ describeOneTableDetails(const char *schemaname,
 						  "t.tgenabled, t.tgisinternal, %s\n"
 						  "FROM pg_catalog.pg_trigger t\n"
 						  "WHERE t.tgrelid = '%s' AND ",
-						  (pset.sversion >= 130000 ?
-						   "(SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass"
-						   " FROM pg_catalog.pg_trigger AS u, "
-						   "      pg_catalog.pg_partition_ancestors(t.tgrelid) AS a"
-						   " WHERE u.tgname = t.tgname AND u.tgrelid = a.relid"
-						   "       AND u.tgparentid = 0) AS parent" :
+						  (pset.sversion >= 130000 ? "\n"
+						   "  (SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass\n"
+						   "   FROM pg_catalog.pg_trigger AS u,\n"
+						   "      pg_catalog.pg_partition_ancestors(t.tgrelid) AS a\n"
+						   "   WHERE u.tgname = t.tgname AND u.tgrelid = a.relid\n"
+						   "        AND u.tgparentid = 0) AS parent" :
 						   "NULL AS parent"),
 						  oid);
 
-- 
2.17.1

>From d866624abbd523afdbc3a539f14c935c42fc345f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Fri, 16 Jul 2021 19:57:00 -0500
Subject: [PATCH 2/2] psql: fix \d for statement triggers with same name on
 partition and its parent

This depends on pg_partition_ancestors() to return its partitions in order:
this partition => parent => ... => root.

[email-masked]
---
 src/bin/psql/describe.c                |  4 ++--
 src/test/regress/expected/triggers.out | 13 +++++++++++++
 src/test/regress/sql/triggers.sql      |  4 ++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index ed9f320b015..8787849e8be 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2996,9 +2996,9 @@ describeOneTableDetails(const char *schemaname,
 						  (pset.sversion >= 130000 ? "\n"
 						   "  (SELECT (NULLIF(a.relid, t.tgrelid))::pg_catalog.regclass\n"
 						   "   FROM pg_catalog.pg_trigger AS u,\n"
-						   "      pg_catalog.pg_partition_ancestors(t.tgrelid) AS a\n"
+						   "      pg_catalog.pg_partition_ancestors(t.tgrelid) WITH ORDINALITY AS a(relid,depth)\n"
 						   "   WHERE u.tgname = t.tgname AND u.tgrelid = a.relid\n"
-						   "        AND u.tgparentid = 0) AS parent" :
+						   "        AND u.tgparentid = 0 ORDER BY depth LIMIT 1) AS parent" :
 						   "NULL AS parent"),
 						  oid);
 
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 5c0e7c2b79e..9278cc07237 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -2122,6 +2122,19 @@ Triggers:
 alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail
 ERROR:  trigger "trg1" for relation "trigpart3" already exists
 drop table trigpart3;
+create trigger samename after delete on trigpart execute function trigger_nothing();
+create trigger samename after delete on trigpart1 execute function trigger_nothing();
+\d trigpart1
+             Table "public.trigpart1"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+ b      | integer |           |          | 
+Partition of: trigpart FOR VALUES FROM (0) TO (1000)
+Triggers:
+    samename AFTER DELETE ON trigpart1 FOR EACH STATEMENT EXECUTE FUNCTION trigger_nothing()
+    trg1 AFTER INSERT ON trigpart1 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON TABLE trigpart
+
 drop table trigpart;
 drop function trigger_nothing();
 --
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 9cb15c21dc3..5e2197e10e2 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -1428,6 +1428,10 @@ create trigger trg1 after insert on trigpart3 for each row execute procedure tri
 alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail
 drop table trigpart3;
 
+create trigger samename after delete on trigpart execute function trigger_nothing();
+create trigger samename after delete on trigpart1 execute function trigger_nothing();
+\d trigpart1
+
 drop table trigpart;
 drop function trigger_nothing();
 
-- 
2.17.1

---

From: Tom Lane <[email-masked]>
Date: Mon, 17 Jan 2022 19:14:34 -0500

> pg_partition_ancestors().

Uh, what?  tgparentid is a trigger OID, not a relation OID.

> This is passing the smoke test.

How hard did you try to break it?  It still seems to me that
this can be fooled by an unrelated trigger with the same tgname.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Mon, 17 Jan 2022 19:50:08 -0500

> this can be fooled by an unrelated trigger with the same tgname.

Hmm ... no, it does work, because we'll stop at the first trigger
with tgparentid = 0, so unrelated triggers further up the partition stack
don't matter.  But this definitely requires commentary.  (And I'm
not too happy with burying such a complicated thing inside a conditional
inside a printf, either.)  Will see about cleaning it up.

			regards, tom lane