Re: BUG #19484: Segmentation fault triggered by FDW

Fujita-san,

On Fri, Jun 19, 2026 at 8:59 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Thu, Jun 18, 2026 at 7:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <etsuro.fujita@gmail.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