Hi,
attached a patch with some cleanup and a couple of test cases. The lookup is
now done with the parentid and the renaming affects detached partitions as well.
The test cases are not perfectly concise, but only add about 0.4 % to the total
runtime of regression tests at my machine. So I didn't bother to much. They
only consist of basic sql tests.
Further feedback appreciated! Thank you!
Regards
Arne
________________________________
From: Arne Roland <a.rol...@index.de>
Sent: Thursday, April 1, 2021 4:38:59 PM
To: Alvaro Herrera
Cc: David Steele; Pg Hackers
Subject: Re: Rename of triggers for partitioned tables
Alvaro Herrera wrote:
> I think the child cannot not have a corresponding trigger. If you try
> to drop the trigger on child, it should say that it cannot be dropped
> because the trigger on parent requires it. So I don't think there's any
> case where altering name of trigger in parent would raise an user-facing
> error. If you can't find the trigger in child, that's a case of catalog
> corruption [...]
Ah, I didn't know that. That changes my reasoning about that. I only knew, that
the alter table ... detach partition ... doesn't detach the child trigger from
the partitioned trigger, but the the attach partition seem to add one. Maybe we
should be able to make sure that there is a one to one correspondence for child
triggers and child partitions.
Currently upon detaching we keep the trigger within the partitioned trigger of
the parent relation. (So the trigger remains in place and can only be dropped
with the parent one.) Only we try to attach the partition again any partitioned
triggers are simply removed.
One idea would be to detach partitioned triggers there in a similar manner we
detach partitioned indexes. That could give above guarantee. (At least if one
would be willing to write a migration for that.) But then we can't tell anymore
where the trigger stems from. That hence would break our attach/detach workflow.
I was annoyed by the inability to drop partitioned triggers from detached
partitions, but it seems I just got a workaround. Ugh.
This is a bit of a sidetrack discussion, but it is related.
> Consider this. You have table p and partition p1. Add trigger t to p,
> and do
> ALTER TRIGGER t ON p1 RENAME TO q;
> What I'm saying is that if you later do
> ALTER TRIGGER t ON ONLY p RENAME TO r;
> then the trigger on parent is changed, and the trigger on child stays q.
> If you later do
> ALTER TRIGGER r ON p RENAME TO s;
> then triggers on both tables end up with name s.
If we get into the mindset of expecting one trigger on the child, we have the
following edge case:
- The trigger is renamed. You seem to favor the silent rename in that case over
raising a notice (or even an error). I am not convinced a notice wouldn't the
better in that case.
- The trigger is outside of partitioned table. That still means that the
trigger might still be in the inheritance tree, but likely isn't. Should we
rename it anyways, or skip that? Should be raise a notice about what we are
doing here? I think that would be helpful to the end user.
> Hmm, ok, maybe this is sufficient, I didn't actually test it. I think
> you do need to add a few test cases to ensure everything is sane. Make
> sure to verify what happens if you have multi-level partitioning
> (grandparent, parent, child) and you ALTER the one in the middle. Also
> things like if parent has two children and one child has multiple
> children.
The test I had somewhere upwards this thread, already tested that.
I am not sure how to add a test for pg_dump though. Can you point me to the
location in pg_regress for pg_dump?
> Also, please make sure that it works correctly with INHERITS (legacy
> inheritance). We probably don't want to cause recursion in that case.
That works, but I will add a test to make sure it stays that way. Thank you for
your input!
Regards
Arne
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 7383d5994e..96cefef7c6 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1326,7 +1326,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))
@@ -1348,12 +1347,8 @@ get_trigger_oid(Oid relid, const char *trigname, bool missing_ok)
return oid;
}
-/*
- * Perform permissions and integrity checks before acquiring a relation lock.
- */
static void
-RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
- void *arg)
+callbackForRenameTrigger(char *relname, Oid relid)
{
HeapTuple tuple;
Form_pg_class form;
@@ -1370,25 +1365,36 @@ RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table, view, or foreign table",
- rv->relname)));
+ relname)));
/* you must own the table to rename one of its triggers */
if (!pg_class_ownercheck(relid, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), rv->relname);
+ aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relid)), relname);
if (!allowSystemTableMods && IsSystemClass(relid, form))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
- rv->relname)));
+ relname)));
ReleaseSysCache(tuple);
}
/*
- * renametrig - changes the name of a trigger on a relation
+ * Perform permissions and integrity checks before acquiring a relation lock.
+ */
+static void
+RangeVarCallbackForRenameTrigger(const RangeVar *rv, Oid relid, Oid oldrelid,
+ void *arg)
+{
+ callbackForRenameTrigger(rv->relname, relid);
+}
+
+/*
+ * 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
@@ -1398,41 +1404,27 @@ 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;
- /*
- * 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],
@@ -1453,34 +1445,48 @@ 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);
+
+ /*
+ * Skip triggers that not relevant. Relevant is the parent trigger as
+ * well as parts of the current distributed trigger.
+ */
+ if (tgparent == 0 && strcmp(stmt->subname, NameStr(trigform->tgname)) || tgparent && trigform->tgparentid != tgparent)
+ continue;
+
+ if (strcmp(stmt->subname, NameStr(trigform->tgname)))
+ {
+ 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);
@@ -1490,29 +1496,118 @@ 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;
}
+/*
+ * renametrig - changes the name of a trigger on a possibly partitioned relation recurisvely
+ *
+ * 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. */
+ if (stmt->relation->inh && relkind == RELKIND_PARTITIONED_TABLE && has_subclass(relid))
+ {
+ ListCell *child;
+ List *children;
+
+ /* Make sure it stays a child, even if it's detached. */
+ children = find_inheritance_children(relid, AccessExclusiveLock, true);
+
+ /*
+ * 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()
@@ -4350,7 +4445,7 @@ GetAfterTriggersStoreSlot(AfterTriggersTableData *table,
/* Create it if not already done. */
if (!table->storeslot)
{
- MemoryContext oldcxt;
+ MemoryContext oldcxt;
/*
* We only need this slot only until AfterTriggerEndQuery, but making
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7ff36bc842..84e1747402 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8851,7 +8851,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/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 7c77c338ce..96269fc2ad 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -394,8 +394,8 @@ RI_FKey_check(TriggerData *trigdata)
* Now check that foreign key exists in PK table
*
* XXX detectNewRows must be true when a partitioned table is on the
- * referenced side. The reason is that our snapshot must be fresh
- * in order for the hack in find_inheritance_children() to work.
+ * referenced side. The reason is that our snapshot must be fresh in
+ * order for the hack in find_inheritance_children() to work.
*/
ri_PerformCheck(riinfo, &qkey, qplan,
fk_rel, pk_rel,
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..0e2e267a05 100644
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -3349,3 +3349,118 @@ 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 some_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 ('some_trigger', 'b', 'something')
+order by tgname, tgrelid::regclass::text;
+ tgrelid | tgname | parent_tgname
+-------------+--------------+---------------
+ grandparent | b |
+ chi | some_trigger | some_trigger
+ cho | some_trigger | some_trigger
+ middle | some_trigger | b
+(4 rows)
+
+--
+-- Check that classical inheritance is unphased by renaming triggers
+--
+create table inh (id int,
+primary key (id));
+alter table middle detach partition chi;
+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..f06586fcae 100644
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -2535,3 +2535,101 @@ 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 some_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 ('some_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));
+
+alter table middle detach partition chi;
+
+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();
