Re: Segfault in RI UPDATE CASCADE on partitioned tables with LIKE+ATTACH child (attnum drift) - Mailing list pgsql-bugs

On Wed, Oct 22, 2025 at 5:29 AM David Rowley <dgrowleyml@gmail.com> wrote:
> On Wed, 22 Oct 2025 at 02:49, Amit Langote <amitlangote09@gmail.com> wrote:
> > Besides test suite being entirely different in my v5, the only change
> > from your v4 was rewording and moving the new comment in
> > ExecGetTriggerResultRel() into its header comment:
> >
> > @@ -1337,6 +1337,12 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
> >   * also provides a way for EXPLAIN ANALYZE to report the runtimes of such
> >   * triggers.)  So we make additional ResultRelInfo's as needed, and save them
> >   * in es_trig_target_relations.
> > + *
> > + * When reusing cached entries from any of these lists, require an exact
> > + * match of ri_RootResultRelInfo and the rootRelInfo passed by the caller.
> > + * This avoids returning an entry created for a different parent context
> > + * and ensures child->parent translation is neither skipped nor applied
> > + * incorrectly.  If no exact match exists, build a new ResultRelInfo.
> >   */
> >  ResultRelInfo *
> >  ExecGetTriggerResultRel(EState *estate, Oid relid,
> > @@ -1347,14 +1353,6 @@ ExecGetTriggerResultRel(EState *estate, Oid relid,
> >      Relation    rel;
> >      MemoryContext oldcontext;
> >
> > -    /*
> > -     * Before creating a new ResultRelInfo, check if we've already made and
> > -     * cached one for this relation.  We must ensure that the given
> > -     * 'rootRelInfo' matches the one stored in the cached ResultRelInfo as
> > -     * trigger handling for partitions can result in mixed requirements for
> > -     * what ri_RootResultRelInfo is set to.
> > -     */
> > -
> >
> > Please take it if you think that's a good change.  That said, I don't
> > have any issue with your v6, code or tests.
>
> The reason I like the comment being within the function body is that I
> didn't see any need to tell the caller to care about this. As far as
> the caller should be concerned, the function should return a correctly
> set up ResultRelInfo. The inner details of how the caching works feels
> like it should be implementation details, and therefore comments about
> that feel better placed inside the function body.

Makes sense.

> > > I also checked to see if there were any changes around what's shown in
> > > the Triggers portion of EXPLAIN ANALYZE for Dmitry's query. Both
> > > patched and unpatched EXPLAIN ANALYZE are the same. I'm not yet clear
> > > on if it's possible something could be different there now for some
> > > other cases that cause additional ResultRelInfos to be added to the
> > > lists.
> >
> > Good point.
> >
> > I made report_triggers() print which list each ResultRelInfo came from
> > (with the attached). It looks like the entries that could have been
> > affected are always in es_trig_target_relations anyway, so the change
> > in ExecGetTriggerResultRel() doesn’t alter what EXPLAIN shows under
> > “Triggers.” I was still a bit surprised, since I expected there might
> > now be an extra entry after we stopped reusing a child entry with a
> > mismatching ri_RootResultRelInfo.
> >
> > I do see child relations listed under “Triggers,” but only when they
> > have trigger events of their own, not when they’re processed as child
> > relations of a root after a CP_UPDATE event. In those cases,
> > afterTriggerInvokeEvents() only passes the instrumentation for the
> > root rel to AfterTriggerExecute(), so the child entries don’t have
> > their ri_TrigInstrument updated and don’t appear in the EXPLAIN
> > output.
>
> That's good to know, so sounds like we're safe from listing any
> additional ResultRelInfos there.

I think so.

> I'm happy to go with v6.

It looks good to me as well.

I also thought about whether the test case could be simplified further
but didn’t come up with anything that still reproduces the failure
cleanly.

I then tried to understand why we end up with an INSERT event on
stages_50_like (queued due to the FK from stages into pipelines)
during the cascaded update on its parent, which creates the rootless
ResultRelInfo in es_trig_target_relations. That same entry was later
reused for the parent’s CP_UPDATE event (queued due to the FK from
builds into stages), leading to the crash. I was curious why we queue
that INSERT event instead of a CP_UPDATE on the parent, since the
latter would have avoided the problem. When I hacked the code to do
that, the crash disappeared -- but that’s not a valid fix, since
FK-side triggers can’t be used with partitioned tables; they assume
heap relations, so queuing CP_UPDATE events for FK-side triggers
doesn’t work in general.

Anyway, just thought to share that here for the record, since it
helped me understand how that rootless ResultRelInfo ends up being
created in the first place.

--
Thanks, Amit Langote