Re: BUG #19484: Segmentation fault triggered by FDW

On Wed, Jun 24, 2026 at 9:00 Amit Langote <amitlangote09(at)gmail(dot)com> wrote:

> On Tue, Jun 23, 2026 at 5:27 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
> > On Mon, Jun 22, 2026 at 5:28 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > > On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
> wrote:
> >
> > > > +-- Runtime pruning of result relations must keep ModifyTable's
> per-relation
> > > > +-- FDW arrays (fdwPrivLists, fdwDirectModifyPlans) aligned with the
> kept
> > > > +-- resultRelations. Otherwise BeginForeignModify() reads the wrong
> > > > +-- fdw_private and segfaults.
> > > >
> > > > This comment isn't 100% correct, because this issue happens with
> > > > DirectModify as well. I think we could expand the comment to mention
> > > > that as well, but the comment is already too much/detailed IMO; I
> > > > don't think we add such a comment for a test case, so how about
> > > > simplifying the comment like this: "Test that direct modify and
> > > > foreign modify work with runtime pruning of result relations (bug
> > > > #19484)"
> > >
> > > I agree that the details shouldn't leak into test code and have been
> > > trying to follow that principle. Also, bug numbers shouldn't be
> > > mentioned because one can always use `git blame` to find them.
> > > However, since different committers have different styles, I'm fine
> > > with it; kept in the attached updated patch.
> >
> > Thanks!
> >
> > > > @@ -1446,6 +1446,12 @@ typedef struct ModifyTableState
> > > > int mt_nrels; /* number of entries in
> resultRelInfo[] */
> > > > ResultRelInfo *resultRelInfo; /* info about target relation(s)
> */
> > > >
> > > > + /*
> > > > + * Re-indexed fdw private data lists, aligned with
> resultRelInfo[] after
> > > > + * pruning
> > > > + */
> > > > + List *mt_fdwPrivLists;
> > > >
> > > > For v18, I think we should put this at the end of the
> ModifyTableState
> > > > struct, to avoid ABI breakage.
> > >
> > > Good point on ABI. Rather than placing it differently per branch, how
> > > about putting mt_fdwPrivLists at the end of the struct alongside
> > > mt_updateColnosLists/mt_mergeActionLists/mt_mergeJoinConditions, which
> > > are the same kind of unpruned-filtered lists? That keeps it ABI-safe
> > > and lets master and 18 share an identical change. I'd fold it into
> > > their existing comment too.
> >
> > That's a good idea! So +1
> >
> > > Updated patch attached.
> >
> > LGTM.
>
> Pushed, thanks for checking.

crake is now red on REL_18:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2026-06-24%2000%3A02%3A03

It reports sizeof(ModifyTableState) grew by 8 bytes.

That’s from adding mt_fdwPrivLists at the end of the struct. Placing it
last keeps existing field offsets stable, which is what extensions reading
the node rely on, but it does grow the struct, which is what the checker
flags. I believe we’ve added trailing struct members in back branches
before for exactly this reason. Is a flagged sizeof increase acceptable
here under that precedent, or do we now want to avoid any flagged ABI
change on a back branch?

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. master
would keep the field as committed, so the branches would differ slightly.

I’m inclined toward the no-field version for REL_18 to keep the farm green,
unless someone would rather keep the same code in both branches and accept
the size change. I’ll hold off briefly for any thoughts before doing that.
- Amit

>