From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Arne Roland <A.Roland@index.de>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Date: Tue, 20 Jul 2021 16:13:21 -0400
Message-ID: <202107202013.xgao3c2m6nht@alvherre.pgsql> (raw)
In-Reply-To: <202107200139.5zceapxcs6z7@alvherre.pgsql>
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/Attachments:v9-0001-Make-ALTER-TRIGGER-recurse-to-partitions.patch (text/x-diff, 15.7K) download
From 0a2a5e92437475162161837001c7aef16f57908e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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.cindex 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.yindex 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.outindex 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.sqlindex 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
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: alvherre@alvh.no-ip.org, A.Roland@index.de, vignesh21@gmail.com, zyu@yugabyte.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Rename of triggers for partitioned tables
In-Reply-To: <202107202013.xgao3c2m6nht@alvherre.pgsql>
* 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