public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Langote <[email protected]>
To: Etsuro Fujita <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: Ayush Tiwari <[email protected]>
Cc: Rafia Sabih <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Amit Langote <[email protected]>
Subject: Re: BUG #19484: Segmentation fault triggered by FDW
Date: Wed, 24 Jun 2026 20:20:29 +0900
Message-ID: <CA+HiwqFd61sXgUWijKg8DfRQDtwYEGSdxuEPDb8rq22rxzxN_g@mail.gmail.com> (raw)
In-Reply-To: <CAPmGK14CFGgCSCor1DHe6GVVQpjj+zDq755K5_vgc32U2+OGYQ@mail.gmail.com>
References: <[email protected]>
<CAJTYsWXY9C3B-7NZw72OKen2L2rZt=c-t6=kjTJzgj=ZaNPe8g@mail.gmail.com>
<[email protected]>
<CA+FpmFc10Dtnr9dr=xqK2cNYQ4-1VuOyvOT7-eqXPmiQJoyPew@mail.gmail.com>
<[email protected]>
<CAJTYsWWUfgCZau2fvrZVruXmC+dyOx45L=TyaGzXdywsywDyHQ@mail.gmail.com>
<[email protected]>
<CA+HiwqHVMY1a5E+6Ei369+RqQ8qKSSgon_C=3yBk-TPJDwZ2nQ@mail.gmail.com>
<CA+HiwqHjUO0uXFZ2dECJiNime6B1FDL5+mJ=XN+1XgzkY=m9DA@mail.gmail.com>
<CAPmGK16GNiwcnAxM3XU=ZTOP_8oiv7_FOUwo5hZp8b+qfv2L7g@mail.gmail.com>
<CA+HiwqEVwEPSE4yMvuwVT5bb+ziMHLfqd_HuiQPAFAixNHbhrQ@mail.gmail.com>
<CA+HiwqHO0Bbi6cyk4F+UGvZEV3NEo7o0YT3-Js7K7hvasoWBVw@mail.gmail.com>
<CAPmGK16fALJqbyR1Oir7-TsN8k_yAR-JcOg5nWOUmDmivTmDtA@mail.gmail.com>
<CAPmGK14Qh_Wb4GNPNkZ1o9=vTcMKShzPNSWa=wNf9vJpg8qT+Q@mail.gmail.com>
<CA+HiwqHRDsYDqJ-7f5P_QpJo=U1BYzMnfDnNOr6Vx3Rw36O0iQ@mail.gmail.com>
<CAPmGK15nR_pePb9QAMViq+AGuWeJSqPnLTJVLsr96aU2TFmcmw@mail.gmail.com>
<CA+HiwqFT0VDOCAdN1bcVZTOF0oBbR4kzLF-OwEcgPOf01me-Kw@mail.gmail.com>
<CAPmGK15nx4a_QkTcbD2BwsDuDPtbS25Pzmq_sc-xM4EitoHaxw@mail.gmail.com>
<CA+HiwqHTsZmceNHxVfmD0VVN-F1eMSuVQYVDWfsz3OSe+qoO7A@mail.gmail.com>
<CA+HiwqEhe7-v5Q0-oOoW3RaO4voYcGK-JfinbYEWXwutDGSOtQ@mail.gmail.com>
<CAMbWs4-hBR6H3AEv1LkcnRt04Q4zgVVUeOqmA6-W=DSP65s3Xw@mail.gmail.com>
<CA+HiwqEbqW_957S9UAEoZJ5aYyBF_Bp+ON+6thQNCySxSwv+Fg@mail.gmail.com>
<CAPmGK14CFGgCSCor1DHe6GVVQpjj+zDq755K5_vgc32U2+OGYQ@mail.gmail.com>
Fujita-san,
On Wed, Jun 24, 2026 at 6:15 PM Etsuro Fujita <[email protected]> wrote:
> On Wed, Jun 24, 2026 at 1:25 PM Amit Langote <[email protected]> wrote:
> > On Wed, Jun 24, 2026 at 11:23 AM Richard Guo <[email protected]> wrote:
> > > On Wed, Jun 24, 2026 at 10:43 AM Amit Langote <[email protected]> wrote:
> > > > If we’d rather avoid it, I have a no-field alternative for REL_18: show_modifytable_info() is the only reader of the re-indexed list, and it can recompute the mapping from node->resultRelations and node->fdwPrivLists, both plan-ordered and untouched by pruning.
> > >
> > > This sounds like a nice solution. Since show_modifytable_info() is
> > > only used for EXPLAIN, I guess the recompute overhead on the
> > > re-indexed list should be fine.
> >
> > Thanks for chiming in, here is the patch to do so.
>
> Thanks for the patch!
>
> + /*
> + * node->fdwPrivLists is indexed by the original, pre-pruning
> + * result relation order and is parallel to node->resultRelations.
> + * Initial pruning may have dropped earlier relations, so the kept
> + * index j need not match the original position; find this
> + * relation's entry by its range table index instead.
> + */
> + forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
> + {
> + if (lfirst_int(lc1) == (int) rti)
> + {
> + fdw_private = (List *) lfirst(lc2);
> + break;
> + }
> + }
>
> I think it's good to skip this for efficiency, when there are no
> pruned result relations.
Thanks for the review. v2 attached implements your suggestion: it
indexes node->fdwPrivLists directly with j when no result relations
were pruned, and only falls back to matching by range table index when
pruning appears to have dropped some.
> Other than that the patch looks good to me.
>
> I didn't know pg_rewrite's use of ModifyTableState, which was
> different than I expected. Sorry for that.
No need to apologize at all. Tom's point about it was new to me too.
I will push v2 to REL_18 tomorrow barring objections.
--
Thanks, Amit Langote
Attachments:
[application/octet-stream] v2-0001-Avoid-ABI-break-in-ModifyTableState-from-the-FDW-.patch (4.5K, ../CA+HiwqFd61sXgUWijKg8DfRQDtwYEGSdxuEPDb8rq22rxzxN_g@mail.gmail.com/2-v2-0001-Avoid-ABI-break-in-ModifyTableState-from-the-FDW-.patch)
download | inline diff:
From 5407f57e9ce7a89946e0fe20a3f6d6cdf21eabd8 Mon Sep 17 00:00:00 2001
From: Amit Langote <[email protected]>
Date: Wed, 24 Jun 2026 20:12:00 +0900
Subject: [PATCH v2] Avoid ABI break in ModifyTableState from the FDW pruning
fix
Commit 1ef917e3a6 fixed the re-indexing of ModifyTable's FDW arrays
when initial runtime pruning removes result relations, but it did so
by adding a new mt_fdwPrivLists field to ModifyTableState. Although
the field was placed at the end of the struct to keep the offsets of
existing fields stable, it still enlarges sizeof(ModifyTableState),
which the ABI compliance check flags on the buildfarm (e.g. crake).
The field existed only so that show_modifytable_info() could recover the
re-indexed fdw_private after executor startup; the executor-side fix in
ExecInitModifyTable() that actually prevents the crash does not depend on
it. Remove the field and have show_modifytable_info() instead look up
each kept relation's fdw_private from the original, pre-pruning
node->fdwPrivLists, which is parallel to node->resultRelations and left
intact by pruning. When nothing was pruned the lookup is a direct index;
otherwise it matches on the range table index.
This is applied to REL_18 only; master keeps the mt_fdwPrivLists field
and is unaffected, so the two diverge slightly here.
Reported on the buildfarm (member crake).
Per a suggestion from Tom Lane.
Reviewed-by: Etsuro Fujita <[email protected]>
Discussion: https://postgr.es/m/CA+HiwqEhe7-v5Q0-oOoW3RaO4voYcGK-JfinbYEWXwutDGSOtQ@mail.gmail.com
---
src/backend/commands/explain.c | 29 +++++++++++++++++++++++++-
src/backend/executor/nodeModifyTable.c | 1 -
src/include/nodes/execnodes.h | 8 +++----
3 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 6d2624e75b8..444178ed89c 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4609,7 +4609,34 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
fdwroutine != NULL &&
fdwroutine->ExplainForeignModify != NULL)
{
- List *fdw_private = (List *) list_nth(mtstate->mt_fdwPrivLists, j);
+ List *fdw_private;
+
+ /*
+ * node->fdwPrivLists is indexed by the original, pre-pruning
+ * result relation order and is parallel to node->resultRelations.
+ * If no result relations were pruned, the kept order matches that
+ * order and we can index it directly with j. Otherwise the kept
+ * index j need not match the original position, so find this
+ * relation's entry by its range table index instead.
+ */
+ if (list_length(node->resultRelations) == mtstate->mt_nrels)
+ fdw_private = (List *) list_nth(node->fdwPrivLists, j);
+ else
+ {
+ Index rti = resultRelInfo->ri_RangeTableIndex;
+ ListCell *lc1;
+ ListCell *lc2;
+
+ fdw_private = NIL;
+ forboth(lc1, node->resultRelations, lc2, node->fdwPrivLists)
+ {
+ if (lfirst_int(lc1) == (int) rti)
+ {
+ fdw_private = (List *) lfirst(lc2);
+ break;
+ }
+ }
+ }
fdwroutine->ExplainForeignModify(mtstate,
resultRelInfo,
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index cac30666663..7c1d0e9588e 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -4770,7 +4770,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate->mt_updateColnosLists = updateColnosLists;
mtstate->mt_mergeActionLists = mergeActionLists;
mtstate->mt_mergeJoinConditions = mergeJoinConditions;
- mtstate->mt_fdwPrivLists = fdwPrivLists;
/*----------
* Resolve the target relation. This is the same as:
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 6677a03caab..409e172bfb6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1453,15 +1453,13 @@ typedef struct ModifyTableState
double mt_merge_deleted;
/*
- * Lists of valid updateColnosLists, mergeActionLists,
- * mergeJoinConditions, and fdwPrivLists. These contain only entries for
- * unpruned relations, filtered from the corresponding lists in
- * ModifyTable.
+ * Lists of valid updateColnosLists, mergeActionLists, and
+ * mergeJoinConditions. These contain only entries for unpruned
+ * relations, filtered from the corresponding lists in ModifyTable.
*/
List *mt_updateColnosLists;
List *mt_mergeActionLists;
List *mt_mergeJoinConditions;
- List *mt_fdwPrivLists;
} ModifyTableState;
/* ----------------
--
2.47.3
view thread (32+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19484: Segmentation fault triggered by FDW
In-Reply-To: <CA+HiwqFd61sXgUWijKg8DfRQDtwYEGSdxuEPDb8rq22rxzxN_g@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox