Re: BUG #19484: Segmentation fault triggered by FDW

Amit-san,

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:

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

+-- 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)"

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

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

That's it.  Sorry for the delay.

Best regards,
Etsuro Fujita