FOR EACH ROW triggers, on partitioend tables, with indexes?
On Wed, Aug 17, 2022 at 09:54:34AM -0500, Justin Pryzby wrote:
But an unfortunate consequence of not fixing the historic issues is that it
precludes the possibility that anyone could be expected to notice if they
introduce more instances of the same problem (as in the first half of these
patches). Then the hole which has already been dug becomes deeper, further
increasing the burden of fixing the historic issues before being able to use
-Wshadow.The first half of the patches fix shadow variables newly-introduced in v15
(including one of my own patches), the rest are fixing the lowest hanging fruit
of the "short list" from COPT=-Wshadow=compatible-localI can't see that any of these are bugs, but it seems like a good goal to move
towards allowing use of the -Wshadow* options to help avoid future errors, as
well as cleanliness and readability (rather than allowing it to get harder to
use -Wshadow).
+Alvaro
You wrote:
|commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96
|Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
|Date: Fri Mar 23 10:48:22 2018 -0300
|
| Allow FOR EACH ROW triggers on partitioned tables
Which added:
1 + partition_recurse = !isInternal && stmt->row &&
2 + rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE;
3 ...
4 + if (partition_recurse)
5 ...
6 + List *idxs = NIL;
7 + List *childTbls = NIL;
8 ...
9 + if (OidIsValid(indexOid))
10 + {
11 + ListCell *l;
12 + List *idxs = NIL;
13 +
14 + idxs = find_inheritance_children(indexOid, ShareRowExclusiveLock);
15 + foreach(l, idxs)
16 + childTbls = lappend_oid(childTbls,
17 + IndexGetRelation(lfirst_oid(l),
18 + false));
19 + }
20 ...
21 + for (i = 0; i < partdesc->nparts; i++)
22 ...
23 + if (OidIsValid(indexOid))
24 ...
25 + forboth(l, idxs, l2, childTbls)
The inner idxs is set at line 12, but the outer idxs being looped over at line
25 is still NIL, because the variable is shadowed.
That would be a memory leak or some other bug, except that it also seems to be
dead code ?
https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html#1166
Is it somwhow possible to call CreateTrigger() to create a FOR EACH ROW
trigger, with an index, and not internally ?
The comments say that a user's CREATE TRIGGER will not have a constraint, so
won't have an index.
* constraintOid is zero when
* executing a user-entered CREATE TRIGGER command.
*
+ * indexOid, if nonzero, is the OID of an index associated with the constraint.
+ * We do nothing with this except store it into pg_trigger.tgconstrindid.See also: <20220817145434.GC26426@telsasoft.com>
Re: shadow variables - pg15 edition
--
Justin
On Sat, 20 Aug 2022 at 09:18, Justin Pryzby <pryzby@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
Attachments:
remove_dead_code_from_CreateTriggerFiringOn.patchtext/plain; charset=US-ASCII; name=remove_dead_code_from_CreateTriggerFiringOn.patchDownload
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 */
On Thu, Sep 01, 2022 at 04:19:37PM +1200, David Rowley wrote:
On Sat, 20 Aug 2022 at 09:18, Justin Pryzby <pryzby@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.
It's possible that extensions or 3rd party code or forks use this, no ?
In that case, it might be "not dead" ..
+ * that ever changes then we'll need to quite code here to find the
quite? write? quire? acquire? quine?
--
Justin
On 2022-Aug-19, Justin Pryzby wrote:
That would be a memory leak or some other bug, except that it also seems to be
dead code ?https://coverage.postgresql.org/src/backend/commands/trigger.c.gcov.html#1166
Is it somwhow possible to call CreateTrigger() to create a FOR EACH ROW
trigger, with an index, and not internally ?
TBH I don't remember this at all anymore.
So apparently the way to get a trigger associated with a relation
(tgconstrrelid) is via CREATE CONSTRAINT TRIGGER, but there doesn't
appear to be a way to have it associated with a specific *index* on that
relation (tgconstrindid). So you're right that it appears to be dead
code.
If the regression tests don't break by removing it, I agree with doing
that.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)
On Thu, 1 Sept 2022 at 20:57, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
So apparently the way to get a trigger associated with a relation
(tgconstrrelid) is via CREATE CONSTRAINT TRIGGER, but there doesn't
appear to be a way to have it associated with a specific *index* on that
relation (tgconstrindid). So you're right that it appears to be dead
code.If the regression tests don't break by removing it, I agree with doing
that.
Thanks for having a look here. Yeah, it was a while ago.
I've pushed a patch to remove the dead code from master. I don't quite
see the sense in removing it in the back branches.
David