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 */
