Re: BUG #19484: Segmentation fault triggered by FDW

Amit-san,

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:

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