Date: Mon, 17 Jan 2022 18:08:24 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, pgsql-hackers@postgresql.org,
	Amit Langote <amitlangote09@gmail.com>
Subject: Re: \d with triggers: more than one row returned by a subquery used
 as an expression
Message-ID: <20220118000824.GC14051@telsasoft.com>
References: <20211217154356.GJ17618@telsasoft.com>
 <20211222210308.GD17618@telsasoft.com>
 <152507.1642456920@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="tR2cvQ13k2aMxNOa"
Content-Disposition: inline
In-Reply-To: <152507.1642456920@sss.pgh.pa.us>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20220118000824.GC14051%40telsasoft.com>
Precedence: bulk


--tR2cvQ13k2aMxNOa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jan 17, 2022 at 05:02:00PM -0500, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> 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/20210717010259.GU20208@telsasoft.com
> >> 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

--tR2cvQ13k2aMxNOa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-psql-Add-newlines-and-spacing-in-trigger-query-for-d.patch"

From 99f8ee921258d9b2e75299e69826004fda3b8919 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
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


--tR2cvQ13k2aMxNOa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0002-psql-fix-d-for-statement-triggers-with-same-name-on-.patch"

From d866624abbd523afdbc3a539f14c935c42fc345f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
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.

20210716193319.GS20208@telsasoft.com
---
 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


--tR2cvQ13k2aMxNOa--