On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote: > On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby <pry...@telsasoft.com> wrote: > > On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote: > > > What happens if you detach the parent? I mean, should the trigger > > > removal recurse to children? > > > > It think it should probably exactly undo what CloneRowTriggersToPartition > > does. > > ..and I guess you're trying to politely say that it didn't. I tried to fix > > in > > v4 - please check if that's right. > > Looks correct to me. Maybe have a test cover that?
I included such a test with the v4 patch. > > > > But if we remove trigger during DETACH, then it's *not* the same as a > > > > trigger > > > > that was defined on the child, and I suggest that in v13 we should make > > > > that > > > > visible. > > > > > > Hmm, interesting point -- whether the trigger is partition or not is > > > important because it affects what happens on detach. I agree that we > > > should make it visible. Is the proposed single bit "PARTITION" good > > > enough, or should we indicate what's the ancestor table that defines the > > > partition? > > > > Yea, it's an obvious thing to do. > > This: > > + "false AS tgisinternal"), > + (pset.sversion >= 13000 ? > + "pg_partition_root(t.tgrelid) AS parent" : > + "'' AS parent"), > + oid); > > > looks wrong, because the actual partition root may not also be the > trigger parent root, for example: Sigh, right. > The following gets the correct parent for me: > > - (pset.sversion >= 13000 ? > - "pg_partition_root(t.tgrelid) AS parent" : > - "'' AS parent"), > + (pset.sversion >= 130000 ? > + "(SELECT relid" > + " FROM pg_trigger, > pg_partition_ancestors(t.tgrelid)" > + " WHERE tgname = t.tgname AND tgrelid = relid" > + " AND tgparentid = 0) AS parent" : > + " null AS parent"), 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. > 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. I split into separate patches since only 0001 should be backpatched (with s/OidIsValid(tgparentid)/isPartitionTrigger/). -- Justin
>From dc24d8fdbe4435feaea9a99fcf3e1e1121cc8950 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Fri, 3 Apr 2020 22:43:26 -0500 Subject: [PATCH v5 1/2] fix detaching tables with inherited row triggers The old behavior is buggy, and the intended behavior was not previously documented. So define the behavior that the trigger is removed if the table is detached. It is an error if a table being attached already has a trigger of the same. This differs from the behavior for indexes and constraints. Should backpatch to v11 with s/OidIsValid(tgparentid)/isPartitionTrigger/. See also: 86f575948c773b0ec5b0f27066e37dd93a7f0a96 Discussion: https://www.postgresql.org/message-id/flat/20200408152412.GZ2228%40telsasoft.com --- doc/src/sgml/ref/create_trigger.sgml | 1 + src/backend/commands/tablecmds.c | 42 ++++++++++++++++++++++++ src/test/regress/expected/triggers.out | 45 ++++++++++++++++++++++++++ src/test/regress/sql/triggers.sql | 21 ++++++++++++ 4 files changed, 109 insertions(+) diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml index bde3a63f90..303881fcfb 100644 --- a/doc/src/sgml/ref/create_trigger.sgml +++ b/doc/src/sgml/ref/create_trigger.sgml @@ -526,6 +526,7 @@ UPDATE OF <replaceable>column_name1</replaceable> [, <replaceable>column_name2</ Creating a row-level trigger on a partitioned table will cause identical triggers to be created in all its existing partitions; and any partitions created or attached later will contain an identical trigger, too. + If the partition is detached from its parent, the trigger is removed. Triggers on partitioned tables may not be <literal>INSTEAD OF</literal>. </para> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 037d457c3d..514c5ff844 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -16797,6 +16797,48 @@ ATExecDetachPartition(Relation rel, RangeVar *name) } table_close(classRel, RowExclusiveLock); + /* + * Drop any ROW triggers inherited from partitioned table. + * This undoes what CloneRowTriggersToPartition did. + */ + { + ScanKeyData skey; + SysScanDesc scan; + HeapTuple trigtup; + Relation tgrel; + + ScanKeyInit(&skey, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber, + F_OIDEQ, ObjectIdGetDatum(RelationGetRelid(partRel))); + tgrel = table_open(TriggerRelationId, RowExclusiveLock); + scan = systable_beginscan(tgrel, TriggerRelidNameIndexId, + true, NULL, 1, &skey); + + while (HeapTupleIsValid(trigtup = systable_getnext(scan))) + { + Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(trigtup); + ObjectAddress object; + + if (!OidIsValid(pg_trigger->tgparentid) || + !pg_trigger->tgisinternal || + !TRIGGER_FOR_ROW(pg_trigger->tgtype)) + continue; + + deleteDependencyRecordsFor(TriggerRelationId, + pg_trigger->oid, + false); + deleteDependencyRecordsFor(RelationRelationId, + pg_trigger->oid, + false); + + CommandCounterIncrement(); + ObjectAddressSet(object, TriggerRelationId, pg_trigger->oid); + performDeletion(&object, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); + } + + systable_endscan(scan); + table_close(tgrel, RowExclusiveLock); + } + /* * Detach any foreign keys that are inherited. This includes creating * additional action triggers. diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index e9da4ef983..0e30029bcf 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2023,6 +2023,51 @@ select tgrelid::regclass, tgname, tgfoid::regproc from pg_trigger ---------+--------+-------- (0 rows) +-- check detach behavior +create trigger trg1 after insert on trigpart for each row execute procedure trigger_nothing(); +\d trigpart3 + Table "public.trigpart3" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +Partition of: trigpart FOR VALUES FROM (2000) TO (3000) +Triggers: + 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" +ERROR: trigger "trg1" for table "trigpart3" does not exist +alter table trigpart detach partition trigpart4; +drop trigger trg1 on trigpart41; -- fail due to "does not exist" +ERROR: trigger "trg1" for table "trigpart41" does not exist +drop table trigpart4; +alter table trigpart attach partition trigpart3 for values from (2000)to(3000); +alter table trigpart detach partition trigpart3; +alter table trigpart attach partition trigpart3 for values from (2000)to(3000); +drop table trigpart3; +select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal from pg_trigger + where tgname~'^trg1' order by 1; + tgrelid | tgname | tgfoid | tgenabled | tgisinternal +-----------+--------+-----------------+-----------+-------------- + trigpart | trg1 | trigger_nothing | O | f + trigpart1 | trg1 | trigger_nothing | O | t +(2 rows) + +create table trigpart3 (like trigpart); +create trigger trg1 after insert on trigpart3 for each row execute procedure trigger_nothing(); +\d trigpart3 + Table "public.trigpart3" + Column | Type | Collation | Nullable | Default +--------+---------+-----------+----------+--------- + a | integer | | | + b | integer | | | +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 +ERROR: trigger "trg1" for relation "trigpart3" already exists +drop table trigpart3; drop table trigpart; drop function trigger_nothing(); -- diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 80ffbb4b02..4b250fcc8d 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1380,6 +1380,27 @@ drop trigger trg1 on trigpart; -- ok, all gone select tgrelid::regclass, tgname, tgfoid::regproc from pg_trigger where tgrelid::regclass::text like 'trigpart%' order by tgrelid::regclass::text; +-- check detach behavior +create trigger trg1 after insert on trigpart for each row execute procedure trigger_nothing(); +\d trigpart3 +alter table trigpart detach partition trigpart3; +drop trigger trg1 on trigpart3; -- fail due to "does not exist" +alter table trigpart detach partition trigpart4; +drop trigger trg1 on trigpart41; -- fail due to "does not exist" +drop table trigpart4; +alter table trigpart attach partition trigpart3 for values from (2000)to(3000); +alter table trigpart detach partition trigpart3; +alter table trigpart attach partition trigpart3 for values from (2000)to(3000); +drop table trigpart3; + +select tgrelid::regclass::text, tgname, tgfoid::regproc, tgenabled, tgisinternal from pg_trigger + where tgname~'^trg1' order by 1; +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 +drop table trigpart3; + drop table trigpart; drop function trigger_nothing(); -- 2.17.0
>From 3b76f4d2546d94d0a35ec28899e431c77dc6101d Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Mon, 20 Apr 2020 13:46:06 -0500 Subject: [PATCH v5 2/2] Make inherited status of triggers visible in psql.. Unlike inherited indexes and constraints, the trigger is removed if the table is detached. Make that visibly apparent. HEAD/v13 only. --- src/bin/psql/describe.c | 17 +++++++++++++++-- src/test/regress/expected/triggers.out | 2 +- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index f05e914b4d..ead4e0bf47 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -2939,14 +2939,21 @@ describeOneTableDetails(const char *schemaname, printfPQExpBuffer(&buf, "SELECT t.tgname, " "pg_catalog.pg_get_triggerdef(t.oid%s), " - "t.tgenabled, %s\n" + "t.tgenabled, %s, %s\n" "FROM pg_catalog.pg_trigger t\n" "WHERE t.tgrelid = '%s' AND ", (pset.sversion >= 90000 ? ", true" : ""), (pset.sversion >= 90000 ? "t.tgisinternal" : pset.sversion >= 80300 ? "t.tgconstraint <> 0 AS tgisinternal" : - "false AS tgisinternal"), oid); + "false AS tgisinternal"), + (pset.sversion >= 130000 ? + "(SELECT a.relid" + " FROM pg_trigger AS u, pg_partition_ancestors(t.tgrelid) AS a" + " WHERE u.tgname = t.tgname AND u.tgrelid = a.relid" + " AND u.tgparentid = 0) AS parent" : + "'' 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" @@ -3062,6 +3069,12 @@ describeOneTableDetails(const char *schemaname, tgdef = usingpos + 9; printfPQExpBuffer(&buf, " %s", tgdef); + + /* Visually distinguish inherited triggers XXX: ROW only? */ + if (!PQgetisnull(result, i, 4)) + 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.out index 0e30029bcf..a760eb15da 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() + trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON TABLE trigpart alter table trigpart detach partition trigpart3; drop trigger trg1 on trigpart3; -- fail due to "does not exist" -- 2.17.0
