On 2021-Jul-19, Alvaro Herrera wrote:
> Well, it does rename a trigger named 'name' on the table 'table_name',
> as well as all its descendant triggers. I guess I am surprised that
> anybody would rename a descendant trigger in the first place. I'm not
> wedded to the decision of removing the NOTICE, though ... are there any
> other votes for that, anyone?
I put it back, mostly because I don't really care and it's easily
removed if people don't want it. (I used a different wording though,
not necessarily final.)
I also realized that if you rename a trigger and the target name is
already occupied, we'd better throw a nicer error message than failure
by violation of a unique constraint; so I moved the check for the name
to within renametrig_internal(). Added a test for this.
Also added a test to ensure that nothing happens with statement
triggers. This doesn't need any new code, because those triggers don't
have tgparentid set.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
>From 0a2a5e92437475162161837001c7aef16f57908e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Tue, 20 Jul 2021 16:00:31 -0400
Subject: [PATCH v9] Make ALTER TRIGGER recurse to partitions
---
src/backend/commands/trigger.c | 197 +++++++++++++++++++------
src/backend/parser/gram.y | 2 +-
src/test/regress/expected/triggers.out | 109 ++++++++++++++
src/test/regress/sql/triggers.sql | 59 ++++++++
4 files changed, 323 insertions(+), 44 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 6d4b7ee92a..0005cf657d 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,17 @@ 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, unless
+ * caller requested not to. Lock all tables upfront, if needed.
*/
+ if (stmt->relation->inh &&
+ 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 +1474,27 @@ 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);
+ /* Rename the trigger on this relation ... */
+ renametrig_internal(tgrel, targetrel, tuple, stmt->newname,
+ stmt->subname);
- CatalogTupleUpdate(tgrel, &tuple->t_self, tuple);
+ /* ... and if it is partitioned, recurse to its partitions */
+ if (stmt->relation->inh &&
+ targetrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ PartitionDesc partdesc = RelationGetPartitionDesc(targetrel, true);
- InvokeObjectPostAlterHook(TriggerRelationId,
- tgoid, 0);
+ for (int i = 0; i < partdesc->nparts; i++)
+ {
+ Oid partitionId = partdesc->oids[i];
- /*
- * 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);
+ renametrig_partition(tgrel, partitionId, trigform->oid,
+ stmt->newname, stmt->subname);
+ }
+ }
}
else
{
@@ -1533,6 +1518,132 @@ 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 newtgform;
+ ScanKeyData key[2];
+ SysScanDesc tgscan;
+
+ /*
+ * 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 */
+ newtgform = (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(newtgform->tgname), expected_name) != 0)
+ ereport(NOTICE,
+ errmsg("renamed trigger \"%s\" on relation \"%s\"",
+ NameStr(newtgform->tgname),
+ RelationGetRelationName(targetrel)));
+
+ namestrcpy(&newtgform->tgname, newname);
+
+ CatalogTupleUpdate(tgrel, &tuple->t_self, tuple);
+
+ InvokeObjectPostAlterHook(TriggerRelationId, newtgform->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/backend/parser/gram.y b/src/backend/parser/gram.y
index 10da5c5c51..26433c529f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8886,7 +8886,7 @@ RenameStmt: ALTER AGGREGATE aggregate_with_argtypes RENAME TO name
n->missing_ok = false;
$$ = (Node *)n;
}
- | ALTER TRIGGER name ON qualified_name RENAME TO name
+ | ALTER TRIGGER name ON relation_expr RENAME TO name
{
RenameStmt *n = makeNode(RenameStmt);
n->renameType = OBJECT_TRIGGER;
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 5254447cf8..9e47bc4d45 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3410,3 +3410,112 @@ 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 view ourtriggers as
+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;
+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 (8);
+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();
+select * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+--------+---------------
+ chi | a | a
+ cho | a | a
+ grandparent | a |
+ middle | a | a
+(4 rows)
+
+alter trigger a on grandparent rename to b;
+select * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+--------+---------------
+ chi | b | b
+ cho | b | b
+ grandparent | b |
+ middle | b | b
+(4 rows)
+
+alter trigger b on only middle rename to something;
+select * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+-----------+---------------
+ chi | b | something
+ cho | b | something
+ grandparent | b |
+ middle | something | b
+(4 rows)
+
+alter trigger something on middle rename to a_trigger;
+NOTICE: renamed trigger "b" on relation "chi"
+NOTICE: renamed trigger "b" on relation "cho"
+select * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+-----------+---------------
+ chi | a_trigger | a_trigger
+ cho | a_trigger | a_trigger
+ middle | a_trigger | b
+ grandparent | b |
+(4 rows)
+
+-- Renaming fails if a trigger with the target name exists in a partition
+create trigger final after insert on cho for each row execute procedure f();
+alter trigger b on grandparent rename to final;
+NOTICE: renamed trigger "a_trigger" on relation "middle"
+ERROR: trigger "final" for relation "cho" already exists
+select * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+-----------+---------------
+ chi | a_trigger | a_trigger
+ cho | a_trigger | a_trigger
+ middle | a_trigger | b
+ grandparent | b |
+ cho | final |
+(5 rows)
+
+-- Renaming 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 * from ourtriggers;
+ tgrelid | tgname | parent_tgname
+-------------+-----------+---------------
+ chi | a_trigger | a_trigger
+ cho | a_trigger | a_trigger
+ middle | a_trigger | b
+ grandparent | b |
+ cho | final |
+ middle | p |
+ grandparent | q |
+(7 rows)
+
+drop view ourtriggers;
+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..41e5172648 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2572,3 +2572,62 @@ 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 view ourtriggers as
+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;
+
+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 (8);
+
+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();
+
+select * from ourtriggers;
+
+alter trigger a on grandparent rename to b;
+select * from ourtriggers;
+
+alter trigger b on only middle rename to something;
+select * from ourtriggers;
+
+alter trigger something on middle rename to a_trigger;
+select * from ourtriggers;
+
+-- Renaming fails if a trigger with the target name exists in a partition
+create trigger final after insert on cho for each row execute procedure f();
+alter trigger b on grandparent rename to final;
+select * from ourtriggers;
+
+-- Renaming 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 * from ourtriggers;
+
+drop view ourtriggers;
+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
