FOR EACH ROW triggers, on partitioend tables, with indexes?

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Tomas Vondra <tomas(dot)vondra(at)postgresql(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Subject: FOR EACH ROW triggers, on partitioend tables, with indexes?
Date: 2022-08-19 21:18:24
Message-ID: 20220819211824.GX26426@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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(at)alvh(dot)no-ip(dot)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(dot)GC26426(at)telsasoft(dot)com>
Re: shadow variables - pg15 edition

--
Justin

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-08-19 21:26:02 Re: use ARM intrinsics in pg_lfind32() where available
Previous Message Alvaro Herrera 2022-08-19 20:53:52 Re: Schema variables - new implementation for Postgres 15