On Tue, Oct 21, 2025 at 4:28 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Tue, 21 Oct 2025 at 02:48, Amit Langote <amitlangote09@gmail.com> wrote:
> > For the fix, I tried a slightly different approach from your patch.
> > Instead of updating the cached entry to always set
> > ri_RootResultRelInfo = rootRelInfo on a cache hit, I made
> > ExecGetTriggerResultRel() avoid reusing a rootless ResultRelInfo when
> > a rooted one is expected. Concretely, I added assertions on the two
> > primary lists and tightened the es_trig_target_relations lookup to
> > only return an entry when ri_RootResultRelInfo == rootRelInfo or the
> > caller’s rootRelInfo is NULL. That prevents the rootless destination
> > built for the INSERT on stages -> pipelines from being reused for the
> > later UPDATE from builds -> stages.
>
> foreach(l, estate->es_trig_target_relations)
> {
> rInfo = (ResultRelInfo *) lfirst(l);
> - if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
> + if (RelationGetRelid(rInfo->ri_RelationDesc) == relid &&
> + (rInfo->ri_RootResultRelInfo == rootRelInfo ||
> + rootRelInfo == NULL))
>
> 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.
For es_opened_result_relations and es_tuple_routing_result_relations,
I kept the looser asserts. These lookups can be made with a NULL
rootRelInfo, as in the initial call within afterTriggerInvokeEvents()
that isn’t assuming a child context. In that case, returning a rooted
entry is harmless since it’s only used to fetch trigger metadata, not
for tuple translation. I didn’t turn those asserts into runtime
conditions, because doing so would prevent legitimate reuse in those
safe contexts.
> > This seems a bit safer since it keeps cached entries immutable and
> > surfaces mismatches via asserts rather than mutating shared state.
>
> I think creating separate ResultRelInfos is probably a safer bet.
> That's what I ended up doing in [1] after posting my initial patch
> (which was intended to highlight the problem area.)
>
> [1] https://github.com/postgres/postgres/compare/master...david-rowley:postgres:fix_resultrelinfo_caching
Ah, good to know. I borrowed a bit from your comment in that commit
to update the function header, so it now briefly explains the
rootRelInfo matching rules and the distinction between the lists.
Updated patch attached.
--
Thanks, Amit Langote
