From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables
Date: Tue, 21 Apr 2020 20:06:48 -0500
Message-ID: <20200422010648.GM3890@telsasoft.com> (raw)
In-Reply-To: <20200421230330.GA23831@alvherre.pgsql>
References: <20200420195740.GE3890@telsasoft.com>
<20200421230330.GA23831@alvherre.pgsql>
On Tue, Apr 21, 2020 at 07:03:30PM -0400, Alvaro Herrera wrote:
> On 2020-Apr-20, Justin Pryzby wrote:
>
> > On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote:
>
> > > Also, how about, for consistency, making the parent table labeling of
> > > the trigger look similar to that for the foreign constraint, so
> > > Triggers:
> > > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION trigfunc()
> >
> > I'll leave that for committer to decide.
>
> Pushed. Many thanks for this!
Thanks for polishing it.
I was just about to convince myself of the merits of doing it Amit's way :)
I noticed a few issues:
- should put \n's around Amit's subquery to make psql -E look pretty;
- maybe should quote the TABLE, like \"%s\" ?
#3 is that *if* we did it Amit's way, I *think* maybe we should show the
parent's triggerdef, not the childs.
It seems strange to me to say "TABLE trigpart .* INSERT ON trigpart3"
- TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
+ TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
--
Justin
Attachments:
[text/x-diff] v6-0001-fixups-c33869cc3bfc42bce822251f2fa1a2a346f86cc5.patch (3.2K, ../20200422010648.GM3890@telsasoft.com/2-v6-0001-fixups-c33869cc3bfc42bce822251f2fa1a2a346f86cc5.patch)
download | inline diff:
From 86ff4e592afe56d2611e22b63fa3f1268b583e58 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 3 Apr 2020 22:43:26 -0500
Subject: [PATCH v6 1/2] fixups: c33869cc3bfc42bce822251f2fa1a2a346f86cc5
---
src/bin/psql/describe.c | 14 +++++++-------
src/test/regress/expected/triggers.out | 2 +-
src/test/regress/sql/triggers.sql | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.cindex 8dca6d8bb4..5ef56f7a9d 100644--- a/src/bin/psql/describe.c+++ b/src/bin/psql/describe.c@@ -2947,12 +2947,12 @@ describeOneTableDetails(const char *schemaname,
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" :+ (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);
if (pset.sversion >= 110000)
@@ -3073,7 +3073,7 @@ describeOneTableDetails(const char *schemaname,
/* Visually distinguish inherited triggers */
if (!PQgetisnull(result, i, 4))
- appendPQExpBuffer(&buf, ", ON TABLE %s",+ appendPQExpBuffer(&buf, ", ON TABLE \"%s\"",
PQgetvalue(result, i, 4));
printTableAddFooter(&cont, buf.data);
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.outindex 5e76b3a47e..4104711c29 100644--- a/src/test/regress/expected/triggers.out+++ b/src/test/regress/expected/triggers.out@@ -2065,7 +2065,7 @@ create trigger trg1 after insert on trigpart3 for each row execute procedure tri
Triggers:
trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
-alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail+alter table trigpart attach partition trigpart3 for values from (2000) to (3000); -- fail
ERROR: trigger "trg1" for relation "trigpart3" already exists
drop table trigpart3;
drop table trigpart;
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sqlindex e228d0a8a5..c359c6d3fa 100644--- a/src/test/regress/sql/triggers.sql+++ b/src/test/regress/sql/triggers.sql@@ -1398,7 +1398,7 @@ select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal
create table trigpart3 (like trigpart);
create trigger trg1 after insert on trigpart3 for each row execute procedure trigger_nothing();
\d trigpart3
-alter table trigpart attach partition trigpart3 FOR VALUES FROM (2000) to (3000); -- fail+alter table trigpart attach partition trigpart3 for values from (2000) to (3000); -- fail
drop table trigpart3;
drop table trigpart;
--
2.17.0
[text/x-diff] v6-0002-show-inherited-triggers-Amit-s-way.patch (3.9K, ../20200422010648.GM3890@telsasoft.com/3-v6-0002-show-inherited-triggers-Amit-s-way.patch)
download | inline diff:
From 4712253f66e03f6666d57a7a7a971a9f00c492c6 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 20 Apr 2020 13:46:06 -0500
Subject: [PATCH v6 2/2] show inherited triggers Amit's way..
..this also updates the query to avoid saying things like:
TABLE trigpart .* INSERT ON trigpart3
---
src/bin/psql/describe.c | 42 +++++++++++++++++---------
src/test/regress/expected/triggers.out | 2 +-
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.cindex 5ef56f7a9d..3b02eab84e 100644--- a/src/bin/psql/describe.c+++ b/src/bin/psql/describe.c@@ -2936,10 +2936,27 @@ describeOneTableDetails(const char *schemaname,
PGresult *result;
int tuples;
- printfPQExpBuffer(&buf,+ if (pset.sversion >= 130000)+ {+ printfPQExpBuffer(&buf,+ "SELECT t.tgname, "+ "pg_catalog.pg_get_triggerdef(COALESCE(u.oid, t.oid), true), "+ "t.tgenabled, t.tgisinternal, a.relid AS othertable\n"+ "FROM pg_catalog.pg_trigger t LEFT JOIN\n"+ "pg_catalog.pg_partition_ancestors(t.tgrelid) AS a\n"+ "ON true LEFT JOIN\n"+ "pg_catalog.pg_trigger AS u ON\n"+ "u.tgname = t.tgname AND u.tgrelid = a.relid\n"+ "WHERE t.tgrelid = '%s' AND "+ "(u.tgparentid = 0 OR a.relid IS NULL) AND \n",+ oid);+ }+ else+ {+ printfPQExpBuffer(&buf,
"SELECT t.tgname, "
"pg_catalog.pg_get_triggerdef(t.oid%s), "
- "t.tgenabled, %s, %s\n"+ "t.tgenabled, %s, NULL AS parent\n"
"FROM pg_catalog.pg_trigger t\n"
"WHERE t.tgrelid = '%s' AND ",
(pset.sversion >= 90000 ? ", true" : ""),
@@ -2947,14 +2964,9 @@ describeOneTableDetails(const char *schemaname,
pset.sversion >= 80300 ?
"t.tgconstraint <> 0 AS tgisinternal" :
"false AS tgisinternal"),
- (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);
+ }+
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"
@@ -3069,12 +3081,12 @@ describeOneTableDetails(const char *schemaname,
if (usingpos)
tgdef = usingpos + 9;
- printfPQExpBuffer(&buf, " %s", tgdef);-- /* Visually distinguish inherited triggers */- if (!PQgetisnull(result, i, 4))- appendPQExpBuffer(&buf, ", ON TABLE \"%s\"",- PQgetvalue(result, i, 4));+ if (PQgetisnull(result, i, 4))+ printfPQExpBuffer(&buf, " %s", tgdef);+ else+ /* Visually distinguish inherited triggers */+ printfPQExpBuffer(&buf, " TABLE \"%s\" TRIGGER %s",+ PQgetvalue(result, i, 4), tgdef);
printTableAddFooter(&cont, buf.data);
}
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.outindex 4104711c29..a1b81f49ff 100644--- a/src/test/regress/expected/triggers.out+++ b/src/test/regress/expected/triggers.out@@ -2033,7 +2033,7 @@ create trigger trg1 after insert on trigpart for each row execute procedure trig
b | integer | | |
Partition of: trigpart FOR VALUES FROM (2000) TO (3000)
Triggers:
- trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON TABLE trigpart+ TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing()
alter table trigpart detach partition trigpart3;
drop trigger trg1 on trigpart3; -- fail due to "does not exist"
--
2.17.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, alvherre@2ndquadrant.com, amitlangote09@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables
In-Reply-To: <20200422010648.GM3890@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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