On 2021-Jul-22, Arne Roland wrote: > Since it is sort of the same problem, I think it might be worthwhile > to address it as well within this patch. Adding two to four ereports > doesn't sound like scope creeping to me, even though it touches > completely different code. I'll look into that as well.
I don't understand what you mean. But here's an updated patch, with the following changes 1. support for ONLY is removed, since evidently the only thing it is good for is introduce inconsistencies 2. recursion to partitioned tables always occurs; no more conditionally on relation->inh. This is sensible because due to point 1 above, inh can no longer be false. 3. renaming a trigger that's not topmost is forbidden. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
>From bac7f7f824ef3622281a4aed0a8e658eed3fa1a8 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Wed, 21 Jul 2021 13:32:24 -0400 Subject: [PATCH v10] Make ALTER TRIGGER RENAME consistent for partitioned tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously renaming clone triggers on partitions was allowed, which is pretty much useless and causes pg_dump to be inconsistent (because the different name is not propagated); and also, when triggers were renamed on partitioned tables, the partitions were not affected. Make the operation recurse to partitions, and also forbid renaming clone triggers. Co-authored-by: Arne Roland <a.rol...@index.de> Co-authored-by: Álvaro Herrera <alvhe...@alvh.no-ip.org> Reviewed-by: Zhihong Yu <z...@yugabyte.com> Discussion: https://postgr.es/m/d0fd7040c2fb4de1a111b9d9ccc45...@index.de --- doc/src/sgml/ref/alter_trigger.sgml | 15 +- src/backend/commands/trigger.c | 215 ++++++++++++++++++++----- src/test/regress/expected/triggers.out | 76 +++++++++ src/test/regress/sql/triggers.sql | 47 ++++++ 4 files changed, 307 insertions(+), 46 deletions(-) diff --git a/doc/src/sgml/ref/alter_trigger.sgml b/doc/src/sgml/ref/alter_trigger.sgml index 43a7da4f0b..cec50e2245 100644 --- a/doc/src/sgml/ref/alter_trigger.sgml +++ b/doc/src/sgml/ref/alter_trigger.sgml @@ -31,9 +31,20 @@ ALTER TRIGGER <replaceable class="parameter">name</replaceable> ON <replaceable <para> <command>ALTER TRIGGER</command> changes properties of an existing - trigger. The <literal>RENAME</literal> clause changes the name of + trigger. + </para> + + <para> + The <literal>RENAME</literal> clause changes the name of the given trigger without otherwise changing the trigger - definition. The <literal>DEPENDS ON EXTENSION</literal> clause marks + definition. + If the table that the trigger is on is a partitioned table, + then corresponding clone triggers in the partitions are + renamed too. + </para> + + <para> + The <literal>DEPENDS ON EXTENSION</literal> clause marks the trigger as dependent on an extension, such that if the extension is dropped, the trigger will automatically be dropped as well. </para> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 6d4b7ee92a..e983b361ea 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -71,6 +71,12 @@ int SessionReplicationRole = SESSION_REPLICATION_ROLE_ORIGIN; static int MyTriggerDepth = 0; /* Local function prototypes */ +static void renametrig_internal(Relation tgrel, Relation targetrel, + HeapTuple trigtup, const char *newname, + const char *expected_name); +static void renametrig_partition(Relation tgrel, Oid partitionId, + Oid parentTriggerOid, const char *newname, + const char *expected_name); static void SetTriggerFlags(TriggerDesc *trigdesc, Trigger *trigger); static bool GetTupleForTrigger(EState *estate, EPQState *epqstate, @@ -1442,38 +1448,16 @@ renametrig(RenameStmt *stmt) targetrel = relation_open(relid, NoLock); /* - * Scan pg_trigger twice for existing triggers on relation. We do this in - * order to ensure a trigger does not exist with newname (The unique index - * on tgrelid/tgname would complain anyway) and to ensure a trigger does - * exist with oldname. - * - * NOTE that this is cool only because we have AccessExclusiveLock on the - * relation, so the trigger set won't be changing underneath us. + * On partitioned tables, this operation recurses to partitions. Lock all + * tables upfront. */ + if (targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + (void) find_all_inheritors(relid, AccessExclusiveLock, NULL); + tgrel = table_open(TriggerRelationId, RowExclusiveLock); /* - * First pass -- look for name conflict - */ - ScanKeyInit(&key[0], - Anum_pg_trigger_tgrelid, - BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(relid)); - ScanKeyInit(&key[1], - Anum_pg_trigger_tgname, - BTEqualStrategyNumber, F_NAMEEQ, - PointerGetDatum(stmt->newname)); - tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, - NULL, 2, key); - if (HeapTupleIsValid(tuple = systable_getnext(tgscan))) - ereport(ERROR, - (errcode(ERRCODE_DUPLICATE_OBJECT), - errmsg("trigger \"%s\" for relation \"%s\" already exists", - stmt->newname, RelationGetRelationName(targetrel)))); - systable_endscan(tgscan); - - /* - * Second pass -- look for trigger existing with oldname and update + * Search for the trigger to modify. */ ScanKeyInit(&key[0], Anum_pg_trigger_tgrelid, @@ -1489,27 +1473,39 @@ renametrig(RenameStmt *stmt) { Form_pg_trigger trigform; - /* - * Update pg_trigger tuple with new tgname. - */ - tuple = heap_copytuple(tuple); /* need a modifiable copy */ trigform = (Form_pg_trigger) GETSTRUCT(tuple); tgoid = trigform->oid; - namestrcpy(&trigform->tgname, - stmt->newname); - - CatalogTupleUpdate(tgrel, &tuple->t_self, tuple); - - InvokeObjectPostAlterHook(TriggerRelationId, - tgoid, 0); - /* - * Invalidate relation's relcache entry so that other backends (and - * this one too!) are sent SI message to make them rebuild relcache - * entries. (Ideally this should happen automatically...) + * If the trigger descends from a trigger on a parent partitioned + * table, reject the rename. We don't allow a trigger in a partition + * to differ in name from that of its parent: that would lead to an + * inconsistency that pg_dump would reproduce. */ - CacheInvalidateRelcache(targetrel); + if (OidIsValid(trigform->tgparentid)) + ereport(ERROR, + errmsg("trigger \"%s\" cannot be renamed", stmt->subname), + errhint("Rename trigger on table \"%s\" instead.", + get_rel_name(get_partition_parent(relid, false)))); + + + /* Rename the trigger on this relation ... */ + renametrig_internal(tgrel, targetrel, tuple, stmt->newname, + stmt->subname); + + /* ... and if it is partitioned, recurse to its partitions */ + if (targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + PartitionDesc partdesc = RelationGetPartitionDesc(targetrel, true); + + for (int i = 0; i < partdesc->nparts; i++) + { + Oid partitionId = partdesc->oids[i]; + + renametrig_partition(tgrel, partitionId, trigform->oid, + stmt->newname, stmt->subname); + } + } } else { @@ -1533,6 +1529,137 @@ renametrig(RenameStmt *stmt) return address; } +/* + * Subroutine for renametrig -- perform the actual work of renaming one + * trigger on one table. + * + * If the trigger has a name different from the expected one, raise a + * NOTICE about it. + */ +static void +renametrig_internal(Relation tgrel, Relation targetrel, HeapTuple trigtup, + const char *newname, const char *expected_name) +{ + HeapTuple tuple; + Form_pg_trigger tgform; + ScanKeyData key[2]; + SysScanDesc tgscan; + + /* If the trigger already has the new name, nothing to do. */ + tgform = (Form_pg_trigger) GETSTRUCT(trigtup); + if (strcmp(NameStr(tgform->tgname), newname) == 0) + return; + + /* + * Before actually trying the rename, search for triggers with the same + * name. The update would fail with an ugly message in that case, and it + * is better to throw a nicer error. + */ + ScanKeyInit(&key[0], + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(targetrel))); + ScanKeyInit(&key[1], + Anum_pg_trigger_tgname, + BTEqualStrategyNumber, F_NAMEEQ, + PointerGetDatum(newname)); + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, + NULL, 2, key); + if (HeapTupleIsValid(tuple = systable_getnext(tgscan))) + ereport(ERROR, + (errcode(ERRCODE_DUPLICATE_OBJECT), + errmsg("trigger \"%s\" for relation \"%s\" already exists", + newname, RelationGetRelationName(targetrel)))); + systable_endscan(tgscan); + + /* + * The target name is free; update the existing pg_trigger tuple with it. + */ + tuple = heap_copytuple(trigtup); /* need a modifiable copy */ + tgform = (Form_pg_trigger) GETSTRUCT(tuple); + + /* + * If the trigger has a name different from what we expected, let the user + * know. (We can proceed anyway, since we must have reached here following + * a tgparentid link.) + */ + if (strcmp(NameStr(tgform->tgname), expected_name) != 0) + ereport(NOTICE, + errmsg("renamed trigger \"%s\" on relation \"%s\"", + NameStr(tgform->tgname), + RelationGetRelationName(targetrel))); + + namestrcpy(&tgform->tgname, newname); + + CatalogTupleUpdate(tgrel, &tuple->t_self, tuple); + + InvokeObjectPostAlterHook(TriggerRelationId, tgform->oid, 0); + + /* + * Invalidate relation's relcache entry so that other backends (and this + * one too!) are sent SI message to make them rebuild relcache entries. + * (Ideally this should happen automatically...) + */ + CacheInvalidateRelcache(targetrel); +} + +/* + * Subroutine for renametrig -- Helper for recursing to partitions when + * renaming triggers on a partitioned table. + */ +static void +renametrig_partition(Relation tgrel, Oid partitionId, Oid parentTriggerOid, + const char *newname, const char *expected_name) +{ + SysScanDesc tgscan; + ScanKeyData key; + HeapTuple tuple; + int found PG_USED_FOR_ASSERTS_ONLY = 0; + + /* + * Given a relation and the OID of a trigger on parent relation, find the + * corresponding trigger in the child and rename that trigger to the given + * name. + */ + ScanKeyInit(&key, + Anum_pg_trigger_tgrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(partitionId)); + tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true, + NULL, 1, &key); + while (HeapTupleIsValid(tuple = systable_getnext(tgscan))) + { + Form_pg_trigger tgform = (Form_pg_trigger) GETSTRUCT(tuple); + Relation partitionRel; + + if (tgform->tgparentid != parentTriggerOid) + continue; /* not our trigger */ + + Assert(found++ <= 0); + + partitionRel = table_open(partitionId, NoLock); + + /* Rename the trigger on this partition */ + renametrig_internal(tgrel, partitionRel, tuple, newname, expected_name); + + /* And if this relation is partitioned, recurse to its partitions */ + if (partitionRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + { + PartitionDesc partdesc = RelationGetPartitionDesc(partitionRel, + true); + + for (int i = 0; i < partdesc->nparts; i++) + { + Oid partitionId = partdesc->oids[i]; + + renametrig_partition(tgrel, partitionId, tgform->oid, newname, + NameStr(tgform->tgname)); + } + } + table_close(partitionRel, NoLock); + } + systable_endscan(tgscan); +} /* * EnableDisableTrigger() diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index 5254447cf8..6ca9154066 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -3410,3 +3410,79 @@ for each statement execute function trigger_function1(); delete from convslot_test_parent; NOTICE: trigger = bdt_trigger, old_table = (111,tutu), (311,tutu) drop table convslot_test_child, convslot_test_parent; +-- Test trigger renaming on partitioned tables +create table grandparent (id int, primary key (id)) partition by range (id); +create table middle partition of grandparent for values from (1) to (10) +partition by range (id); +create table chi partition of middle for values from (1) to (5); +create table cho partition of middle for values from (6) to (10); +create function f () returns trigger as +$$ begin return new; end; $$ +language plpgsql; +create trigger a after insert on grandparent +for each row execute procedure f(); +alter trigger a on grandparent rename to b; +select tgrelid::regclass, tgname, +(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname +from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent')) +order by tgname, tgrelid::regclass::text; + tgrelid | tgname | parent_tgname +-------------+--------+--------------- + chi | b | b + cho | b | b + grandparent | b | + middle | b | b +(4 rows) + +alter trigger a on only grandparent rename to b; -- ONLY not supported +ERROR: syntax error at or near "only" +LINE 1: alter trigger a on only grandparent rename to b; + ^ +alter trigger b on middle rename to c; -- can't rename trigger on partition +ERROR: trigger "b" cannot be renamed +HINT: Rename trigger on table "grandparent" instead. +create trigger c after insert on middle +for each row execute procedure f(); +alter trigger b on grandparent rename to c; +ERROR: trigger "c" for relation "middle" already exists +-- Rename cascading does not affect statement triggers +create trigger p after insert on grandparent for each statement execute function f(); +create trigger p after insert on middle for each statement execute function f(); +alter trigger p on grandparent rename to q; +select tgrelid::regclass, tgname, +(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname +from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent')) +order by tgname, tgrelid::regclass::text; + tgrelid | tgname | parent_tgname +-------------+--------+--------------- + chi | b | b + cho | b | b + grandparent | b | + middle | b | b + chi | c | c + cho | c | c + middle | c | + middle | p | + grandparent | q | +(9 rows) + +drop table grandparent; +-- Trigger renaming does not recurse on legacy inheritance +create table parent (a int); +create table child () inherits (parent); +create trigger parenttrig after insert on parent +for each row execute procedure f(); +create trigger parenttrig after insert on child +for each row execute procedure f(); +alter trigger parenttrig on parent rename to anothertrig; +\d+ child + Table "public.child" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + a | integer | | | | plain | | +Triggers: + parenttrig AFTER INSERT ON child FOR EACH ROW EXECUTE FUNCTION f() +Inherits: parent + +drop table parent, child; +drop function f(); diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index 7b73ee20a1..fb94eca3ed 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -2572,3 +2572,50 @@ for each statement execute function trigger_function1(); delete from convslot_test_parent; drop table convslot_test_child, convslot_test_parent; + +-- Test trigger renaming on partitioned tables +create table grandparent (id int, primary key (id)) partition by range (id); +create table middle partition of grandparent for values from (1) to (10) +partition by range (id); +create table chi partition of middle for values from (1) to (5); +create table cho partition of middle for values from (6) to (10); +create function f () returns trigger as +$$ begin return new; end; $$ +language plpgsql; +create trigger a after insert on grandparent +for each row execute procedure f(); + +alter trigger a on grandparent rename to b; +select tgrelid::regclass, tgname, +(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname +from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent')) +order by tgname, tgrelid::regclass::text; +alter trigger a on only grandparent rename to b; -- ONLY not supported +alter trigger b on middle rename to c; -- can't rename trigger on partition +create trigger c after insert on middle +for each row execute procedure f(); +alter trigger b on grandparent rename to c; + +-- Rename cascading does not affect statement triggers +create trigger p after insert on grandparent for each statement execute function f(); +create trigger p after insert on middle for each statement execute function f(); +alter trigger p on grandparent rename to q; +select tgrelid::regclass, tgname, +(select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname +from pg_trigger where tgrelid in (select relid from pg_partition_tree('grandparent')) +order by tgname, tgrelid::regclass::text; + +drop table grandparent; + +-- Trigger renaming does not recurse on legacy inheritance +create table parent (a int); +create table child () inherits (parent); +create trigger parenttrig after insert on parent +for each row execute procedure f(); +create trigger parenttrig after insert on child +for each row execute procedure f(); +alter trigger parenttrig on parent rename to anothertrig; +\d+ child + +drop table parent, child; +drop function f(); -- 2.30.2
