Re: BUG #19484: Segmentation fault triggered by FDW

Fujita-san,

On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Thu, Jun 18, 2026 at 7:57 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> >> Actually, I've just started reviewing the patch. I'll let you know
> >> the results (if any) by tomorrow.
>
> > Ah, thanks for the heads up.
>
> Here are my review comments:

Thanks a lot for the review.

> @@ -5167,6 +5176,15 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
>
> returningLists = lappend(returningLists, returningList);
> }
> + if (node->fdwPrivLists)
> + {
> + List *fdwPrivList = (List *)
> list_nth(node->fdwPrivLists, i);
> +
> + fdwPrivLists = lappend(fdwPrivLists, fdwPrivList);
> + }
>
> As node->fdwPrivLists is always created, the if-test is useless, so I
> removed it like the attached. Also, this is nitpicking but, in the
> attached I moved a comment added above
> ("fdwPrivLists/fdwDirectModifyPlans are re-indexed to match
> resultRelations") to a more appropriate place, and did some minor
> editorialization.

Looks good.

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

> +prepare fdw_part_upd2(int) as
> + update fdw_part_update set b = b + random()::int * 0 + 1 where a = $1
> + returning tableoid::regclass, a, b;
> +execute fdw_part_upd2(2);
> +explain (analyze, verbose, costs off, timing off, summary off)
> + execute fdw_part_upd2(2);
>
> I think it's expensive to do explain with the analyze option enabled,
> just for conforming the plan. To save cycles, I removed the option.
> (As the test case for DirectModify was missing the explain test, I
> added it, for consistency.) The test cases are placed in the "test
> tuple routing for foreign-table partitions" section, but they are
> basic ones to test writable foreign tables, so I moved them to the
> "test writable foreign table stuff" section.

Sounds good.

> @@ -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 it. Sorry for the delay.

No problem.

Updated patch attached.

--
Thanks, Amit Langote