Re: Segfault in RI UPDATE CASCADE on partitioned tables with LIKE+ATTACH child (attnum drift)

On Wed, 22 Oct 2025 at 02:49, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Tue, Oct 21, 2025 at 8:22 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> > On Tue, 21 Oct 2025 at 22:12, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Oct 21, 2025 at 2:20 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> > > > 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.
> >
> > I did miss that. You didn't mention it in the email and I didn't look
> > down far enough to see them.
>
> Ok, sorry about that.
>
> > > 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. The CTE test seems worthwhile since it can reuse the same
> > tables and results in using the es_opened_result_relations List. I've
> > included it. I also tidied up the tests a bit more. I looked at your
> > v5 test case and saw it hadn't gone through the same simplification
> > process as I put my version through (making tables non-partitioned
> > when they don't need to be to trigger the issue), so what's in v6 is
> > my version.
>
> Ah, yes, the 3rd table doesn't need to be partitioned.
>
> > I've attached v6. If you have other proposed changes, would you be
> > able to send them as a diff based on my patch? We seem to have got
> > some split-brain
>
> Sorry about that.
>
> > and the patches have diverged a little and that
> > resulted in some of my v4 changes being lost.
>
> 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.

> > 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'm happy to go with v6.

David