From 0b83c47216b0fe71a8a0e880eb550a09f06dad38 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Tue, 21 Apr 2020 13:57:00 -0400 Subject: [PATCH] Fix detaching partitions with cloned row triggers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a partition is detached, any triggers that had been cloned from its parent were not properly disentangled from its parent triggers. This resulted in triggers that could not be dropped because they depended on the trigger in the trigger in the no-longer-parent table: ALTER TABLE t DETACH PARTITION t1; DROP TRIGGER trig ON t1; ERROR: cannot drop trigger trig on table t1 because trigger trig on table t requires it HINT: You can drop trigger trig on table t instead. Moreover the table can no longer be re-attached to its parent, because the trigger name is already taken: ALTER TABLE t ATTACH PARTITION t1 FOR VALUES FROM (1)TO(2); ERROR: trigger "trig" for relation "t1" already exists The former is a bug introduced in commit 86f575948c77. (The latter is not necessarily a bug, but it makes the bug more uncomfortable.) To avoid the complexity that would be needed to tell whether the trigger has a local definition that has to be merged with the one coming from the parent table, establish the behavior that the trigger is removed when the table is detached. Backpatch to pg11. Author: Justin Pryzby Reviewed-by: Amit Langote Reviewed-by: Álvaro Herrera Discussion: https://www.postgresql.org/message-id/flat/20200408152412.GZ2228@telsasoft.com --- doc/src/sgml/ref/create_trigger.sgml | 1 + src/backend/commands/tablecmds.c | 64 ++++++++++++++++++++++++++ src/test/regress/expected/triggers.out | 45 ++++++++++++++++++ src/test/regress/sql/triggers.sql | 21 +++++++++ 4 files changed, 131 insertions(+) diff --git a/doc/src/sgml/ref/create_trigger.sgml b/doc/src/sgml/ref/create_trigger.sgml index 6514ffc6ae..55d53445c4 100644 --- a/doc/src/sgml/ref/create_trigger.sgml +++ b/doc/src/sgml/ref/create_trigger.sgml @@ -524,6 +524,7 @@ UPDATE OF column_name1 [, column_name2AFTER. diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 28996f607c..49d7059d53 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -494,6 +494,7 @@ static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, List *partConstraint, bool validate_default); static void CloneRowTriggersToPartition(Relation parent, Relation partition); +static void DropClonedTriggersFromPartition(Oid partitionId); static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name); static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel, RangeVar *name); @@ -15656,6 +15657,9 @@ ATExecDetachPartition(Relation rel, RangeVar *name) } heap_close(classRel, RowExclusiveLock); + /* Drop any triggers that were cloned on creation/attach. */ + DropClonedTriggersFromPartition(RelationGetRelid(partRel)); + /* * Detach any foreign keys that are inherited. This includes creating * additional action triggers. @@ -15718,6 +15722,66 @@ ATExecDetachPartition(Relation rel, RangeVar *name) return address; } +/* + * DropClonedTriggersFromPartition + * subroutine for ATExecDetachPartition to remove any triggers that were + * cloned to the partition when it was created-as-partition or attached. + * This undoes what CloneRowTriggersToPartition did. + */ +static void +DropClonedTriggersFromPartition(Oid partitionId) +{ + ScanKeyData skey; + SysScanDesc scan; + HeapTuple trigtup; + Relation tgrel; + ObjectAddresses *objects; + + objects = new_object_addresses(); + + /* + * Scan pg_trigger to search for all triggers on this rel. + */ + ScanKeyInit(&skey, Anum_pg_trigger_tgrelid, BTEqualStrategyNumber, + F_OIDEQ, ObjectIdGetDatum(partitionId)); + tgrel = heap_open(TriggerRelationId, RowExclusiveLock); + scan = systable_beginscan(tgrel, TriggerRelidNameIndexId, + true, NULL, 1, &skey); + while (HeapTupleIsValid(trigtup = systable_getnext(scan))) + { + Oid trigoid = HeapTupleGetOid(trigtup); + ObjectAddress trig; + + /* Ignore triggers that weren't cloned */ + if (!isPartitionTrigger(trigoid)) + continue; + + /* + * This is ugly, but necessary: remove the dependency markings on the + * trigger so that it can be removed. + */ + deleteDependencyRecordsForClass(TriggerRelationId, trigoid, + TriggerRelationId, + DEPENDENCY_INTERNAL_AUTO); + deleteDependencyRecordsForClass(TriggerRelationId, trigoid, + RelationRelationId, + DEPENDENCY_INTERNAL_AUTO); + + /* remember this trigger to remove it below */ + ObjectAddressSet(trig, TriggerRelationId, trigoid); + add_exact_object_address(&trig, objects); + } + + /* make the dependency removal visible to the deletion below */ + CommandCounterIncrement(); + performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); + + /* done */ + free_object_addresses(objects); + systable_endscan(scan); + heap_close(tgrel, RowExclusiveLock); +} + /* * Before acquiring lock on an index, acquire the same lock on the owning * table. diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index cd9bdc8c4b..8f7e9a872c 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -2025,6 +2025,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 PROCEDURE 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 PROCEDURE 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 b0cac89b74..3e9a4d98ce 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -1387,6 +1387,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();