pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed\d with triggers: more than one row returned by a subquery used as an expression
6+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2022-01-18 90e0f9fd8c 3886785b4c 9007d4ea77 Fix psql \d's query for identifying parent triggers.
* \d with triggers: more than one row returned by a subquery used as an expression
@ 2021-12-17 15:43 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2021-12-17 15:43 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@postgresql.org, Amit Langote <amitlangote09@gmail.com>
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.
On Fri, Jul 16, 2021 at 08:02:59PM -0500, Justin Pryzby wrote:
> On Fri, Jul 16, 2021 at 06:01:12PM -0400, Alvaro Herrera wrote:
> > On 2021-Jul-16, Justin Pryzby wrote:
> > > CREATE TABLE p(i int) PARTITION BY RANGE(i);
> > > CREATE TABLE p1 PARTITION OF p FOR VALUES FROM (1)TO(2);
> > > CREATE FUNCTION foo() returns trigger LANGUAGE plpgsql AS $$begin end$$;
> > > CREATE TRIGGER x AFTER DELETE ON p1 EXECUTE FUNCTION foo();
> > > CREATE TRIGGER x AFTER DELETE ON p EXECUTE FUNCTION foo();
> >
> > Hmm, interesting -- those statement triggers are not cloned, so what is
> > going on here is just that the psql query to show them is tripping on
> > its shoelaces ... I'll try to find a fix.
> >
> > I *think* the problem is that the query matches triggers by name and
> > parent/child relationship; we're missing to ignore triggers by tgtype.
> > It's not great design that tgtype is a bitmask of unrelated flags ...
>
> I see it's the subquery Amit wrote and proposed here:
> https://www.postgresql.org/message-id/CA+HiwqEiMe0tCOoPOwjQrdH5fxnZccMR7oeW=f9FmgszJQbgFg@mail.gmail...
>
> .. and I realize that I've accidentally succeeded in breaking what I first
> attempted to break 15 months ago:
>
> On Mon, Apr 20, 2020 at 02:57:40PM -0500, Justin Pryzby wrote:
> > I'm happy to see that this doesn't require a recursive cte, at least.
> > I was trying to think how to break it by returning multiple results or results
> > out of order, but I think that can't happen.
>
> If you assume that pg_partition_ancestors returns its results in order, I think
> you can fix it by adding LIMIT 1. Otherwise I think you need a recursive CTE,
> as I'd feared.
>
> Note also that I'd sent a patch to add newlines, to make psql -E look pretty.
> v6-0001-fixups-c33869cc3bfc42bce822251f2fa1a2a346f86cc5.patch
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: \d with triggers: more than one row returned by a subquery used as an expression
@ 2021-12-22 21:03 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2021-12-22 21:03 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@postgresql.org, Amit Langote <amitlangote09@gmail.com>
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.
Attachments: download all (2 files, .tar.gz)
0001-psql-fix-d-for-statement-triggers-with-same-name-on-.patch (text/x-diff, 3.0K) download
From 1c7282f5c06197e178bd23c50a226a148cb66bc8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 16 Jul 2021 19:57:00 -0500
Subject: [PATCH 1/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 | 2 +-
src/test/regress/expected/triggers.out | 13 +++++++++++++
src/test/regress/sql/triggers.sql | 4 ++++
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 42bad9281ed..7ad10fa1a3f 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3012,7 +3012,7 @@ describeOneTableDetails(const char *schemaname,
" 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" :
+ " AND u.tgparentid = 0 LIMIT 1) AS parent" :
"NULL AS parent"),
oid);
if (pset.sversion >= 110000)
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 9d529e949f4..44c8cdf206f 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 eaab642950b..f644a64ca7d 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.0
0002-psql-Add-newlines-and-spacing-in-trigger-query-for-d.patch (text/x-diff, 1.5K) download
From 543badde5a9bc536d5072cdafd4947a46bfb3efa Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH 2/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 7ad10fa1a3f..a82c945a9a2 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3007,12 +3007,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 LIMIT 1) 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 LIMIT 1) AS parent" :
"NULL AS parent"),
oid);
if (pset.sversion >= 110000)
--
2.17.0
* Re: \d with triggers: more than one row returned by a subquery used as an expression
@ 2022-01-17 22:02 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2022-01-17 22:02 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers@postgresql.org, Amit Langote <amitlangote09@gmail.com>
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. 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?
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: \d with triggers: more than one row returned by a subquery used as an expression
@ 2022-01-18 00:08 Justin Pryzby <pryzby@telsasoft.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Justin Pryzby @ 2022-01-18 00:08 UTC (permalink / raw)
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>
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
Attachments: download all (2 files, .tar.gz)
0001-psql-Add-newlines-and-spacing-in-trigger-query-for-d.patch (text/x-diff, 1.5K) download
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
0002-psql-fix-d-for-statement-triggers-with-same-name-on-.patch (text/x-diff, 3.2K) download
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
* Re: \d with triggers: more than one row returned by a subquery used as an expression
@ 2022-01-18 00:14 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2022-01-18 00:14 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers@postgresql.org, Amit Langote <amitlangote09@gmail.com>
Justin Pryzby <pryzby@telsasoft.com> writes:
> On Mon, Jan 17, 2022 at 05:02:00PM -0500, Tom Lane wrote:
>> 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().
Uh, what? tgparentid is a trigger OID, not a relation OID.
> Is there any reason why WITH ORDINALITY can't work ?
> 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: \d with triggers: more than one row returned by a subquery used as an expression
@ 2022-01-18 00:50 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2022-01-18 00:50 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-hackers@postgresql.org, Amit Langote <amitlangote09@gmail.com>
I wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
>> Is there any reason why WITH ORDINALITY can't work ?
>> 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.
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
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-01-18 00:50 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-12-17 15:43 \d with triggers: more than one row returned by a subquery used as an expression Justin Pryzby <pryzby@telsasoft.com>
2021-12-22 21:03 ` Justin Pryzby <pryzby@telsasoft.com>
2022-01-17 22:02 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-01-18 00:08 ` Justin Pryzby <pryzby@telsasoft.com>
2022-01-18 00:14 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-01-18 00:50 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox