From 3886785b4ce59147b92319e7204ab64a9d8801cb Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 17 Jan 2022 21:18:49 -0500 Subject: [PATCH] Fix psql \d's query for identifying parent triggers. The original coding (from c33869cc3) failed with "more than one row returned by a subquery used as an expression" if there were unrelated triggers of the same tgname on parent partitioned tables. (That's possible because statement-level triggers don't get inherited.) Fix by applying LIMIT 1 after sorting the candidates by inheritance level. Also, wrap the subquery in a CASE so that we don't have to execute it at all when the trigger is visibly non-inherited. Aside from saving some cycles, this avoids the need for a confusing and undocumented NULLIF(). While here, tweak the format of the emitted query to look a bit nicer for "psql -E", and add some explanation of this subquery, because it badly needs it. Report and patch by Justin Pryzby (with some editing by me). Back-patch to v13 where the faulty code came in. Discussion: https://postgr.es/m/20211217154356.GJ17618@telsasoft.com --- src/bin/psql/describe.c | 45 +++++++++++++++++++------- src/test/regress/expected/triggers.out | 14 ++++++++ src/test/regress/sql/triggers.sql | 5 +++ 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index 48348750ee..db767d78f0 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -3250,22 +3250,45 @@ describeOneTableDetails(const char *schemaname, printfPQExpBuffer(&buf, "SELECT t.tgname, " "pg_catalog.pg_get_triggerdef(t.oid%s), " - "t.tgenabled, %s, %s\n" - "FROM pg_catalog.pg_trigger t\n" - "WHERE t.tgrelid = '%s' AND ", + "t.tgenabled, %s,\n", (pset.sversion >= 90000 ? ", true" : ""), (pset.sversion >= 90000 ? "t.tgisinternal" : pset.sversion >= 80300 ? "t.tgconstraint <> 0 AS tgisinternal" : - "false AS tgisinternal"), - (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" : - "NULL AS parent"), + "false AS tgisinternal")); + + /* + * Detect whether each trigger is inherited, and if so, get the name + * of the topmost table it's inherited from. We have no easy way to + * do that pre-v13, for lack of the tgparentid column. Even with + * tgparentid, a straightforward search for the topmost parent would + * require a recursive CTE, which seems unduly expensive. We cheat a + * bit by assuming parent triggers will match by tgname; then, joining + * with pg_partition_ancestors() allows the planner to make use of + * pg_trigger_tgrelid_tgname_index if it wishes. We ensure we find + * the correct topmost parent by stopping at the first-in-partition- + * ancestry-order trigger that has tgparentid = 0. (There might be + * unrelated, non-inherited triggers with the same name further up the + * stack, so this is important.) + */ + if (pset.sversion >= 130000) + appendPQExpBufferStr(&buf, + " CASE WHEN t.tgparentid != 0 THEN\n" + " (SELECT u.tgrelid::pg_catalog.regclass\n" + " FROM pg_catalog.pg_trigger AS u,\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\n" + " ORDER BY a.depth LIMIT 1)\n" + " END AS parent\n"); + else + appendPQExpBufferStr(&buf, " NULL AS parent\n"); + + appendPQExpBuffer(&buf, + "FROM pg_catalog.pg_trigger t\n" + "WHERE t.tgrelid = '%s' AND ", oid); + if (pset.sversion >= 110000) appendPQExpBufferStr(&buf, "(NOT t.tgisinternal OR (t.tgisinternal AND t.tgenabled = 'D') \n" " OR EXISTS (SELECT 1 FROM pg_catalog.pg_depend WHERE objid = t.oid \n" diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 5254447cf8..1d9a25f22d 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2122,6 +2122,20 @@ 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; +-- check display of unrelated triggers +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 7b73ee20a1..2f4516e044 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1428,6 +1428,11 @@ 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; +-- check display of unrelated triggers +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();