From: Arne Roland <A.Roland@index.de>
To: David Steele <david@pgmasters.net>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Date: Mon, 29 Mar 2021 09:38:12 +0000
Message-ID: <e93f8494c6314b58ad52b553b0e50779@index.de> (raw)
In-Reply-To: <63c8be480eda4ef28d17a019e79841e2@index.de>
References: <20210115222641.GA26284@alvherre.pgsql>
<0e245ca1-bb16-2416-a9cb-7255907227d7@pgmasters.net>
<63c8be480eda4ef28d17a019e79841e2@index.de>
Attached a rebased patch with the suggested "ONLY ON" change.
Regards
Arne
________________________________
From: Arne Roland <A.Roland@index.de>
Sent: Monday, March 29, 2021 9:37:53 AM
To: David Steele; Alvaro Herrera
Cc: Pg Hackers
Subject: Re: Rename of triggers for partitioned tables
David Steele wrote:
> Arne, thoughts on Álvaro's comments?
>
> Marking this patch as Waiting for Author.
Thank you for the reminder. I was to absorbed by other tasks. I should be more present in the upcoming weeks.
> I think you did not register the patch in commitfest, so I did that for
> you: https://commitfest.postgresql.org/32/2943/
Thank you! I should have done that.
> As you say, triggers on children don't necessarily have to have the same
> name as on parent; this already happens when the trigger is renamed in
> the child table but not on parent. In that situation the search on the
> child will fail, which will cause the whole thing to fail I think.
>
> We now have the column pg_trigger.tgparentid, and I think it would be
> better (more reliable) to search for the trigger in the child by the
> tgparentid column instead, rather than by name.
The last patch already checks tgparentid and errors out if either of those do not match.
The reasoning behind that was, that I don't see a clear reasonable way to handle this scenario.
If the user choose to rename the child trigger, I am at a loss what to do.
I thought to pass it back to the user to solve the situation instead of simply ignoring the users earlier rename.
Silently renaming sounded a bit presumptuous to me, even though I don't care that much either way.
Do you think silently renaming is better than yielding an error?
> Also, I think it would be good to have
> ALTER TRIGGER .. ON ONLY parent RENAME TO ..
> to avoid recursing to children. This seems mostly pointless, but since
> we've allowed changing the name of the trigger in children thus far,
> then we've implicitly made it supported to have triggers that are named
> differently. (And it's not entirely academic, since the trigger name
> determines firing order.)
If it would be entirely academic, I wouldn't have noticed the whole thing in the first place.
The on only variant sounds like a good idea to me. Even though hardly worth any efford, it seems simple enough. I will work on that.
> Alternatively to this last point, we could decide to disallow renaming
> of triggers on children (i.e. if trigger has tgparentid set, then
> renaming is disallowed). I don't have a problem with that, but it would
> have to be an explicit decision to take.
I rejected that idea, because I was unsure what to do with migrations. If we would be discussing this a few years back, this would have been likely my vote, but I don't see it happening now.
Regards
Arne
Attachments:recursive_tgrename.4.patch (text/x-patch, 7.4K) downloaddiff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 7383d5994e..14e3edcc5c 100644--- a/src/backend/commands/trigger.c+++ b/src/backend/commands/trigger.c@@ -1348,12 +1348,7 @@ 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)+static void 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,26 @@ 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;
- /*- * 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],
@@ -1463,6 +1454,7 @@ renametrig(RenameStmt *stmt)
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)))
@@ -1475,7 +1467,12 @@ renametrig(RenameStmt *stmt)
tuple = heap_copytuple(tuple); /* need a modifiable copy */
trigform = (Form_pg_trigger) GETSTRUCT(tuple);
tgoid = trigform->oid;
-+ if (tgparent != 0 && tgparent != trigform->tgparentid) {+ ereport(ERROR,+ (errcode(ERRCODE_UNDEFINED_OBJECT),+ errmsg("trigger \"%s\" for table \"%s\" is not dependent on the trigger on \"%s\"",+ stmt->subname, RelationGetRelationName(targetrel), stmt->relation->relname)));+ }
namestrcpy(&trigform->tgname,
stmt->newname);
@@ -1513,6 +1510,74 @@ renametrig(RenameStmt *stmt)
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;+ Oid newtgoid;+ ObjectAddress tgaddress;+ /*+ * 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);++ /*+ * The last call of renametrig or renametrigHelper already locked this relation.+ */+ tgaddress = renameonlytrig(stmt, tgrel, relid, tgoid);+ newtgoid = tgaddress.objectId;++ /*+ * In case we have a partioned table, we traverse them and rename every dependend+ * trigger of the same name or error out, if it doesn't exist.+ */+ if (stmt->relation->inh && has_subclass(relid))+ {+ ListCell *child;+ List *children;++ /* Make sure it stays a child. */+ children = find_inheritance_children(relid, AccessExclusiveLock, false);++ /*+ * 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, newtgoid, 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 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/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,
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, david@pgmasters.net, alvherre@alvh.no-ip.org, pgsql-hackers@lists.postgresql.org
Subject: Re: Rename of triggers for partitioned tables
In-Reply-To: <e93f8494c6314b58ad52b553b0e50779@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