From: Arne Roland <A.Roland@index.de>
To: vignesh C <vignesh21@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Date: Thu, 15 Jul 2021 12:43:20 +0000
Message-ID: <6e6491608f694ca488a5d7c404c4b98d@index.de> (raw)
In-Reply-To: <CALDaNm02Z-22O6CBCih_u4dB+M_0GMo=rkngws_bJagXbJGKXA@mail.gmail.com>
References: <7c64cc3c9f1c4c1ca3e1ed5a456d4088@index.de>
<202106261636.6g4befbdrfpn@alvherre.pgsql>
<CALDaNm02Z-22O6CBCih_u4dB+M_0GMo=rkngws_bJagXbJGKXA@mail.gmail.com>
Hello Vignesh,
thank you for your interest!
From: vignesh C <vignesh21@gmail.com>
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
Attachments:recursive_tgrename.07.patch (text/x-patch, 17.0K) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 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.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/include/commands/trigger.h b/src/include/commands/trigger.hindex 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.outindex 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.sqlindex 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();
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: A.Roland@index.de, vignesh21@gmail.com, zyu@yugabyte.com, alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Rename of triggers for partitioned tables
In-Reply-To: <6e6491608f694ca488a5d7c404c4b98d@index.de>
* 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