Hello Vignesh,
thank you for your interest!
From: vignesh C <[email protected]>
Sent: Thursday, July 15, 2021 14:08
To: Arne Roland
Cc: Zhihong Yu; Alvaro Herrera; Pg Hackers
Subject: Re: Rename of triggers for partitioned tables
> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".
Please let me know, whether the attached patch works for you.
Regards
Arne
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 952c8d582a..e02da5e52f 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1327,7 +1327,6 @@ get_trigger_oid(Oid relid, const char *trigname, bool missing_ok)
tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
NULL, 2, skey);
-
tup = systable_getnext(tgscan);
if (!HeapTupleIsValid(tup))
@@ -1387,10 +1386,11 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
}
/*
- * renametrig - changes the name of a trigger on a relation
+ * renameonlytrig - changes the name of a trigger on a relation
*
* trigger name is changed in trigger catalog.
* No record of the previous name is kept.
+ * This function assumes that the row is already locked.
*
* get proper relrelation from relation catalog (if not arg)
* scan trigger catalog
@@ -1400,41 +1400,28 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
* update row in catalog
*/
ObjectAddress
-renametrig(RenameStmt *stmt)
+renameonlytrig(RenameStmt *stmt, Relation tgrel, Oid relid, Oid tgparent)
{
Oid tgoid;
Relation targetrel;
- Relation tgrel;
HeapTuple tuple;
SysScanDesc tgscan;
ScanKeyData key[2];
- Oid relid;
ObjectAddress address;
+ int matched;
+ int stmtTgnameMatches;
- /*
- * Look up name, check permissions, and acquire lock (which we will NOT
- * release until end of transaction).
- */
- relid = RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
- 0,
- RangeVarCallbackForRenameTrigger,
- NULL);
/* Have lock already, so just need to build relcache entry. */
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.
- */
- tgrel = table_open(TriggerRelationId, RowExclusiveLock);
-
- /*
* First pass -- look for name conflict
*/
ScanKeyInit(&key[0],
@@ -1455,34 +1442,50 @@ renametrig(RenameStmt *stmt)
systable_endscan(tgscan);
/*
- * Second pass -- look for trigger existing with oldname and update
+ * Second pass -- look for trigger existing and update if pgparent is
+ * matching
*/
ScanKeyInit(&key[0],
Anum_pg_trigger_tgrelid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(relid));
- ScanKeyInit(&key[1],
- Anum_pg_trigger_tgname,
- BTEqualStrategyNumber, F_NAMEEQ,
- PointerGetDatum(stmt->subname));
+
tgscan = systable_beginscan(tgrel, TriggerRelidNameIndexId, true,
- NULL, 2, key);
- if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
+ NULL, 1, key);
+ matched = 0;
+ while (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
{
Form_pg_trigger trigform;
+ trigform = (Form_pg_trigger) GETSTRUCT(tuple);
+
+ stmtTgnameMatches = strcmp(stmt->subname, NameStr(trigform->tgname));
+ /*
+ * Skip triggers that not relevant. Relevant is the parent trigger as
+ * well as parts of the current distributed trigger.
+ */
+ if (tgparent == 0 && stmtTgnameMatches
+ || tgparent && trigform->tgparentid != tgparent)
+ continue;
+
+ if (stmtTgnameMatches)
+ {
+ ereport(NOTICE,
+ (errmsg("trigger \"%s\" for table \"%s\" was not named \"%s\", renaming anyways",
+ NameStr(trigform->tgname), RelationGetRelationName(targetrel), stmt->subname)));
+ }
+
/*
* 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);
+ tgoid = trigform->oid;
+
InvokeObjectPostAlterHook(TriggerRelationId,
tgoid, 0);
@@ -1492,29 +1495,122 @@ renametrig(RenameStmt *stmt)
* entries. (Ideally this should happen automatically...)
*/
CacheInvalidateRelcache(targetrel);
+ matched++;
}
- else
+
+ systable_endscan(tgscan);
+
+ /*
+ * There always should be exactly one matching trigger on the child
+ * partition. If there isn't fail with an error.
+ */
+ if (!matched)
{
ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("trigger \"%s\" for table \"%s\" does not exist",
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("trigger \"%s\" for child table \"%s\" does not exist",
+ stmt->subname, RelationGetRelationName(targetrel))));
+ }
+ if (matched > 1)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("multiple triggers match \"%s\" for table \"%s\", this might indicate a data corruption",
stmt->subname, RelationGetRelationName(targetrel))));
}
ObjectAddressSet(address, TriggerRelationId, tgoid);
- systable_endscan(tgscan);
-
- table_close(tgrel, RowExclusiveLock);
/*
* Close rel, but keep exclusive lock!
*/
+ relation_close(tgrel, NoLock);
relation_close(targetrel, NoLock);
return address;
}
+/*
+ * renametrigHelper - changes the name of a trigger on a possibly partitioned relation recursively
+ *
+ * alter name of parent trigger using renameonlytrig
+ * recursive over children and change their names in the same manner
+ *
+ */
+ObjectAddress
+renametrigHelper(RenameStmt *stmt, Oid tgoid, Oid relid)
+{
+
+ Relation tgrel;
+ ObjectAddress tgaddress;
+ HeapTuple tuple;
+ char relkind;
+
+ /*
+ * NOTE that this is cool only because we have AccessExclusiveLock on the
+ * relations, so neither the trigger set nor the table itself will be
+ * changing underneath us.
+ */
+ tgrel = table_open(TriggerRelationId, RowExclusiveLock);
+
+ /*
+ * The last call of renametrig or renametrigHelper already locked this
+ * relation.
+ */
+ tgaddress = renameonlytrig(stmt, tgrel, relid, tgoid);
+
+ /*
+ * In case we have a partioned table, we traverse them and rename every
+ * dependend trigger, unless ON ONLY was specified.
+ */
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+ relkind = ((Form_pg_class) GETSTRUCT(tuple))->relkind;
+ ReleaseSysCache(tuple); /* We are still holding the
+ * AccessExclusiveLock on the table of the trigger. */
+ if (stmt->relation->inh && relkind == RELKIND_PARTITIONED_TABLE && has_subclass(relid))
+ {
+ ListCell *child;
+ List *children;
+
+ /*
+ * Make sure it stays a child. We assume that there are no parts of
+ * the trigger in detached partitions, since they should have been
+ * deleted in that case.
+ */
+ children = find_inheritance_children(relid, AccessExclusiveLock);
+
+ /*
+ * find_inheritance_children doesn't work recursively. we check every
+ * layer individually to ensure that the trigger of the child is
+ * matching in case it's a partitioned table itself.
+ */
+ foreach(child, children)
+ {
+ Oid childrelid = lfirst_oid(child);
+
+ if (childrelid == relid)
+ continue;
+ renametrigHelper(stmt, tgaddress.objectId, childrelid);
+ }
+ }
+ return tgaddress;
+}
+
+ObjectAddress
+renametrig(RenameStmt *stmt)
+{
+ /*
+ * Look up name, check permissions, and acquire lock (which we will NOT
+ * release until end of transaction).
+ */
+ return renametrigHelper(stmt, 0,
+ RangeVarGetRelidExtended(stmt->relation, AccessExclusiveLock,
+ 0,
+ RangeVarCallbackForRenameTrigger,
+ NULL));
+
+}
/*
* 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/include/commands/trigger.h b/src/include/commands/trigger.h
index 9e557cfbce..8dac23d980 100644
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -158,6 +158,10 @@ extern ObjectAddress CreateTrigger(CreateTrigStmt *stmt, const char *queryString
extern void RemoveTriggerById(Oid trigOid);
extern Oid get_trigger_oid(Oid relid, const char *name, bool missing_ok);
+extern ObjectAddress renameonlytrig(RenameStmt *stmt, Relation tgrel, Oid relid, Oid tgparent);
+
+extern ObjectAddress renametrigHelper(RenameStmt *stmt, Oid tgoid, Oid relid);
+
extern ObjectAddress renametrig(RenameStmt *stmt);
extern void EnableDisableTrigger(Relation rel, const char *tgname,
diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index e8af9a9589..e004794ae2 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3349,3 +3349,134 @@ 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;
+--
+-- build a nested partition scheme for testing
+--
+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 (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 tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('a')
+order by tgname, tgrelid::regclass::text;
+ 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 tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('a', 'b')
+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 b on only middle rename to something;
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('something', 'b')
+order by tgname, tgrelid::regclass::text;
+ 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: trigger "b" for table "chi" was not named "something", renaming anyways
+NOTICE: trigger "b" for table "cho" was not named "something", renaming anyways
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('a_trigger', 'b', 'something')
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+-------------+-----------+---------------
+ chi | a_trigger | a_trigger
+ cho | a_trigger | a_trigger
+ middle | a_trigger | b
+ grandparent | b |
+(4 rows)
+
+--
+-- Check that classical inheritance is unphased by renaming triggers
+--
+create table inh (id int,
+primary key (id));
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+---------+-----------+---------------
+ chi | a_trigger | a_trigger
+(1 row)
+
+alter table middle detach partition chi;
+alter trigger a_trigger on middle rename to some_trigger;
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+---------+--------+---------------
+(0 rows)
+
+alter table chi inherit inh;
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+---------+--------+---------------
+(0 rows)
+
+create trigger unrelated
+before update of id on inh
+for each row
+execute procedure f();
+alter trigger unrelated on inh rename to some_trigger;
+alter trigger some_trigger on inh rename to trigger_name;
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('some_trigger', 'unrelated', 'trigger_name')
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+---------+--------------+---------------
+ cho | some_trigger | some_trigger
+ middle | some_trigger | b
+ inh | trigger_name |
+(3 rows)
+
+-- cleanup
+drop table grandparent;
+drop table chi;
+drop table inh;
+drop function f();
diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index b50f500045..3218b9b36b 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2535,3 +2535,108 @@ for each statement execute function trigger_function1();
delete from convslot_test_parent;
drop table convslot_test_child, convslot_test_parent;
+
+--
+-- build a nested partition scheme for testing
+--
+
+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 (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 tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('a')
+order by tgname, tgrelid::regclass::text;
+
+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 tgname in ('a', 'b')
+order by tgname, tgrelid::regclass::text;
+
+alter trigger b on only middle rename to something;
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('something', 'b')
+order by tgname, tgrelid::regclass::text;
+
+alter trigger something on middle rename to a_trigger;
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('a_trigger', 'b', 'something')
+order by tgname, tgrelid::regclass::text;
+
+--
+-- Check that classical inheritance is unphased by renaming triggers
+--
+
+create table inh (id int,
+primary key (id));
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+
+alter table middle detach partition chi;
+
+alter trigger a_trigger on middle rename to some_trigger;
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+
+alter table chi inherit inh;
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgrelid = 'chi'::regclass
+order by tgname, tgrelid::regclass::text;
+
+create trigger unrelated
+before update of id on inh
+for each row
+execute procedure f();
+
+alter trigger unrelated on inh rename to some_trigger;
+
+alter trigger some_trigger on inh rename to trigger_name;
+
+select tgrelid::regclass, tgname, (select tgname from pg_trigger tr where tr.oid = pg_trigger.tgparentid) parent_tgname
+from pg_trigger where tgname in ('some_trigger', 'unrelated', 'trigger_name')
+order by tgname, tgrelid::regclass::text;
+
+-- cleanup
+
+drop table grandparent;
+drop table chi;
+drop table inh;
+drop function f();
