On Sat, 20 Aug 2022 at 09:18, Justin Pryzby <pry...@telsasoft.com> wrote: > Is it somwhow possible to call CreateTrigger() to create a FOR EACH ROW > trigger, with an index, and not internally ?
I've been looking over this and I very much agree that the code looks very broken. As for whether this is dead code or not, I've been looking at that too... At trigger.c:1147 we have: if (partition_recurse). partition_recurse can only ever be true if isInternal == false per trigger.c:367's "partition_recurse = !isInternal && stmt->row &&". isInternal is a parameter to the function. Also, the code in question only triggers when the indexOid parameter is a valid oid. So it should just be a matter of looking for usages of CreateTriggerFiringOn() which pass isInternal as false and pass a valid indexOid. There seems to be no direct calls doing this, but we do also call this function via CreateTrigger() and I can see only 1 call to CreateTrigger() that passes isInternal as false, but that explicitly passes indexOid as InvalidOid, so this code looks very much dead to me. Alvaro, any objections to just ripping this out? aka, the attached. I've left an Assert() in there to ensure we notice if we're ever to start calling CreateTriggerFiringOn() with isInternal == false with a valid indexOid. David
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 7661e004a9..08f9a307fd 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -1147,8 +1147,6 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, if (partition_recurse) { PartitionDesc partdesc = RelationGetPartitionDesc(rel, true); - List *idxs = NIL; - List *childTbls = NIL; int i; MemoryContext oldcxt, perChildCxt; @@ -1158,53 +1156,23 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, ALLOCSET_SMALL_SIZES); /* - * When a trigger is being created associated with an index, we'll - * need to associate the trigger in each child partition with the - * corresponding index on it. + * We don't currently expect to be called with a valid indexOid. If + * that ever changes then we'll need to quite code here to find the + * corresponding child index. */ - if (OidIsValid(indexOid)) - { - ListCell *l; - List *idxs = NIL; - - idxs = find_inheritance_children(indexOid, ShareRowExclusiveLock); - foreach(l, idxs) - childTbls = lappend_oid(childTbls, - IndexGetRelation(lfirst_oid(l), - false)); - } + Assert(!OidIsValid(indexOid)); oldcxt = MemoryContextSwitchTo(perChildCxt); /* Iterate to create the trigger on each existing partition */ for (i = 0; i < partdesc->nparts; i++) { - Oid indexOnChild = InvalidOid; - ListCell *l; - ListCell *l2; CreateTrigStmt *childStmt; Relation childTbl; Node *qual; childTbl = table_open(partdesc->oids[i], ShareRowExclusiveLock); - /* Find which of the child indexes is the one on this partition */ - if (OidIsValid(indexOid)) - { - forboth(l, idxs, l2, childTbls) - { - if (lfirst_oid(l2) == partdesc->oids[i]) - { - indexOnChild = lfirst_oid(l); - break; - } - } - if (!OidIsValid(indexOnChild)) - elog(ERROR, "failed to find index matching index \"%s\" in partition \"%s\"", - get_rel_name(indexOid), - get_rel_name(partdesc->oids[i])); - } - /* * Initialize our fabricated parse node by copying the original * one, then resetting fields that we pass separately. @@ -1224,7 +1192,7 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, CreateTriggerFiringOn(childStmt, queryString, partdesc->oids[i], refRelOid, - InvalidOid, indexOnChild, + InvalidOid, InvalidOid, funcoid, trigoid, qual, isInternal, true, trigger_fires_when); @@ -1235,8 +1203,6 @@ CreateTriggerFiringOn(CreateTrigStmt *stmt, const char *queryString, MemoryContextSwitchTo(oldcxt); MemoryContextDelete(perChildCxt); - list_free(idxs); - list_free(childTbls); } /* Keep lock on target rel until end of xact */