Thread: 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-local
>
> I 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
Attachment
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