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

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.

> 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.

--
Thanks, Amit Langote