On Tue, Oct 21, 2025 at 2:20 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Tue, 21 Oct 2025 at 14:28, Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > On Tue, Oct 21, 2025 at 4:28 AM David Rowley <dgrowleyml@gmail.com> wrote:
> > > I don't understand the || rootRelInfo == NULL part. This would break
> > > if we first created the ResultRelInfo with a parent then asked for
> > > another one with no parent.
> >
> > You are right, the || rootRelInfo == NULL check in
> > es_trig_target_relations was too permissive, it could indeed return a
> > rooted entry when the caller explicitly passed NULL. I’ve updated the
> > patch so that es_trig_target_relations now requires strict equality of
> > ri_RootResultRelInfo.
>
> Now I don't understand why you left the Asserts with the rootRelInfo
> == NULL check. If the ri_RootResultRelInfo changes compared to the
> cached one in the es_opened_result_relations or
> es_tuple_routing_result_relations then we're subseptable to the very
> same bug we're aiming to fix here.
Hmm, I had assumed non-NULL mismatches could not occur within one
EState, but with multiple ModifyTableStates (e.g., a CTE update plus
the main update) the same child relid can be opened under different
parent ResultRelInfos. For example:
WITH cte AS (UPDATE parted_cp_update_bug_2_p1 SET a = a) UPDATE
parted_cp_update_bug_1 SET a = 2 WHERE a = 1;
In that case a cached entry’s ri_RootResultRelInfo can differ from the
rootRelInfo passed in, so returning a relid match is unsafe (crashes
under v3!). I agree the assert is not appropriate even ignoring the
NULL clause. IOW, I agree with folding ri_RootResultRelInfo ==
rootRelInfo into the condition across all three lists (your #2).
> I've attached a v4 patch which does #2, adds tests and fixes the
> outdated header comment in ExecGetTriggerResultRel().
Not sure if you missed it, but I had added tests in v2/v3 mirroring
Dmitry's case.
In the attached v5, I’ve updated the test case to use the no-op CTE I
mentioned above, trimmed the test case code a bit, and modified it to
use a new schema like the surrounding tests. I also updated the
comment in ExecGetTriggerResultRel() as you did, and moved the
additional-check explanation into the header comment.
--
Thanks, Amit Langote
