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.
As far as I see it, there's two options:
1) Use Assert(rInfo->ri_RootResultRelInfo == rootRelInfo) for the
es_opened_result_relations and es_tuple_routing_result_relations; or
2) Treat all 3 lists the same and put "&& rInfo->ri_RootResultRelInfo
== rootRelInfo" into the "if" check.
I was a bit uncertain if #1 can be done as I wasn't sure if we could
have the ResultRelInfo we need for a partition movement originating
from a trigger already in the es_opened_result_relations list, but I
played around for a bit and I came up with the following which will
Assert fail if we did the Asserts for #1.
begin;
create table pt (a int primary key, b int references pt (a) on update
cascade on delete cascade) partition by list(a);
create table pt1 partition of pt for values in(1);
create table pt2 partition of pt for values in(2);
insert into pt values(1,null);
update pt set b = 1;
update pt set b = 2;
rollback;
I'm pretty certain that #2 is the correct answer.
I've attached a v4 patch which does #2, adds tests and fixes the
outdated header comment in ExecGetTriggerResultRel().
David
