public inbox for [email protected]
help / color / mirror / Atom feedFrom: Etsuro Fujita <[email protected]>
To: Amit Langote <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: Ayush Tiwari <[email protected]>
Cc: Rafia Sabih <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Amit Langote <[email protected]>
Subject: Re: BUG #19484: Segmentation fault triggered by FDW
Date: Fri, 19 Jun 2026 20:59:39 +0900
Message-ID: <CAPmGK15nR_pePb9QAMViq+AGuWeJSqPnLTJVLsr96aU2TFmcmw@mail.gmail.com> (raw)
In-Reply-To: <CA+HiwqHRDsYDqJ-7f5P_QpJo=U1BYzMnfDnNOr6Vx3Rw36O0iQ@mail.gmail.com>
References: <[email protected]>
<CAJTYsWXY9C3B-7NZw72OKen2L2rZt=c-t6=kjTJzgj=ZaNPe8g@mail.gmail.com>
<[email protected]>
<CA+FpmFc10Dtnr9dr=xqK2cNYQ4-1VuOyvOT7-eqXPmiQJoyPew@mail.gmail.com>
<[email protected]>
<CAJTYsWWUfgCZau2fvrZVruXmC+dyOx45L=TyaGzXdywsywDyHQ@mail.gmail.com>
<[email protected]>
<CA+HiwqHVMY1a5E+6Ei369+RqQ8qKSSgon_C=3yBk-TPJDwZ2nQ@mail.gmail.com>
<CA+HiwqHjUO0uXFZ2dECJiNime6B1FDL5+mJ=XN+1XgzkY=m9DA@mail.gmail.com>
<CAPmGK16GNiwcnAxM3XU=ZTOP_8oiv7_FOUwo5hZp8b+qfv2L7g@mail.gmail.com>
<CA+HiwqEVwEPSE4yMvuwVT5bb+ziMHLfqd_HuiQPAFAixNHbhrQ@mail.gmail.com>
<CA+HiwqHO0Bbi6cyk4F+UGvZEV3NEo7o0YT3-Js7K7hvasoWBVw@mail.gmail.com>
<CAPmGK16fALJqbyR1Oir7-TsN8k_yAR-JcOg5nWOUmDmivTmDtA@mail.gmail.com>
<CAPmGK14Qh_Wb4GNPNkZ1o9=vTcMKShzPNSWa=wNf9vJpg8qT+Q@mail.gmail.com>
<CA+HiwqHRDsYDqJ-7f5P_QpJo=U1BYzMnfDnNOr6Vx3Rw36O0iQ@mail.gmail.com>
Amit-san,
On Thu, Jun 18, 2026 at 7:57 PM Amit Langote <[email protected]> wrote:
> On Thu, Jun 18, 2026 at 19:55 Etsuro Fujita <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-Re-index-ModifyTable-FDW-arrays-when-pruning-resu-efujita.patch (9.1K, ../CAPmGK15nR_pePb9QAMViq+AGuWeJSqPnLTJVLsr96aU2TFmcmw@mail.gmail.com/2-v2-0001-Re-index-ModifyTable-FDW-arrays-when-pruning-resu-efujita.patch)
download | inline diff:
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e90289e4ab1..13853b8b720 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7197,6 +7197,70 @@ RESET enable_material;
DROP FOREIGN TABLE remt2;
DROP TABLE loct1;
DROP TABLE loct2;
+-- Test that direct modify and foreign modify work with runtime pruning of
+-- result relations (bug #19484)
+create table fdw_part_update (a int not null, b int) partition by list (a);
+create table fdw_part_update_p1 partition of fdw_part_update for values in (1);
+create table fdw_part_update_remote (a int not null, b int);
+create foreign table fdw_part_update_p2 partition of fdw_part_update
+ for values in (2)
+ server loopback options (table_name 'fdw_part_update_remote');
+insert into fdw_part_update_p1 values (1, 10);
+insert into fdw_part_update_remote values (2, 20);
+set plan_cache_mode = force_generic_plan;
+-- Check DirectModify case
+prepare fdw_part_upd(int) as
+ update fdw_part_update set b = b + 1 where a = $1
+ returning tableoid::regclass, a, b;
+explain (verbose, costs off)
+ execute fdw_part_upd(2);
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------------------------------
+ Update on public.fdw_part_update
+ Output: (fdw_part_update_1.tableoid)::regclass, fdw_part_update_1.a, fdw_part_update_1.b
+ Foreign Update on public.fdw_part_update_p2 fdw_part_update_2
+ -> Append
+ Subplans Removed: 1
+ -> Foreign Update on public.fdw_part_update_p2 fdw_part_update_2
+ Remote SQL: UPDATE public.fdw_part_update_remote SET b = (b + 1) WHERE ((a = $1::integer)) RETURNING a, b
+(7 rows)
+
+execute fdw_part_upd(2);
+ tableoid | a | b
+--------------------+---+----
+ fdw_part_update_p2 | 2 | 21
+(1 row)
+
+deallocate fdw_part_upd;
+-- Check ForeignModify case
+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;
+explain (verbose, costs off)
+ execute fdw_part_upd2(2);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------------------------------------------------------
+ Update on public.fdw_part_update
+ Output: (fdw_part_update_1.tableoid)::regclass, fdw_part_update_1.a, fdw_part_update_1.b
+ Foreign Update on public.fdw_part_update_p2 fdw_part_update_2
+ Remote SQL: UPDATE public.fdw_part_update_remote SET b = $2 WHERE ctid = $1 RETURNING a, b
+ -> Append
+ Subplans Removed: 1
+ -> Foreign Scan on public.fdw_part_update_p2 fdw_part_update_2
+ Output: ((fdw_part_update_2.b + ((random())::integer * 0)) + 1), fdw_part_update_2.tableoid, fdw_part_update_2.ctid, fdw_part_update_2.*
+ Remote SQL: SELECT a, b, ctid FROM public.fdw_part_update_remote WHERE ((a = $1::integer)) FOR UPDATE
+(9 rows)
+
+execute fdw_part_upd2(2);
+ tableoid | a | b
+--------------------+---+----
+ fdw_part_update_p2 | 2 | 22
+(1 row)
+
+deallocate fdw_part_upd2;
+reset plan_cache_mode;
+drop table fdw_part_update;
+drop table fdw_part_update_remote;
-- ===================================================================
-- test check constraints
-- ===================================================================
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index dfc58beb0d2..697c4a92e2d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1778,6 +1778,40 @@ DROP FOREIGN TABLE remt2;
DROP TABLE loct1;
DROP TABLE loct2;
+-- Test that direct modify and foreign modify work with runtime pruning of
+-- result relations (bug #19484)
+create table fdw_part_update (a int not null, b int) partition by list (a);
+create table fdw_part_update_p1 partition of fdw_part_update for values in (1);
+create table fdw_part_update_remote (a int not null, b int);
+create foreign table fdw_part_update_p2 partition of fdw_part_update
+ for values in (2)
+ server loopback options (table_name 'fdw_part_update_remote');
+insert into fdw_part_update_p1 values (1, 10);
+insert into fdw_part_update_remote values (2, 20);
+set plan_cache_mode = force_generic_plan;
+
+-- Check DirectModify case
+prepare fdw_part_upd(int) as
+ update fdw_part_update set b = b + 1 where a = $1
+ returning tableoid::regclass, a, b;
+explain (verbose, costs off)
+ execute fdw_part_upd(2);
+execute fdw_part_upd(2);
+deallocate fdw_part_upd;
+
+-- Check ForeignModify case
+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;
+explain (verbose, costs off)
+ execute fdw_part_upd2(2);
+execute fdw_part_upd2(2);
+deallocate fdw_part_upd2;
+
+reset plan_cache_mode;
+drop table fdw_part_update;
+drop table fdw_part_update_remote;
+
-- ===================================================================
-- test check constraints
-- ===================================================================
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 112c17b0d64..a40d03d35f3 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -4821,7 +4821,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors,
fdwroutine != NULL &&
fdwroutine->ExplainForeignModify != NULL)
{
- List *fdw_private = (List *) list_nth(node->fdwPrivLists, j);
+ List *fdw_private = (List *) list_nth(mtstate->mt_fdwPrivLists, j);
fdwroutine->ExplainForeignModify(mtstate,
resultRelInfo,
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 33a6735f08d..846dc516b43 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -5108,6 +5108,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
List *updateColnosLists = NIL;
List *mergeActionLists = NIL;
List *mergeJoinConditions = NIL;
+ List *fdwPrivLists = NIL;
+ Bitmapset *fdwDirectModifyPlans = NULL;
ResultRelInfo *resultRelInfo;
List *arowmarks;
ListCell *l;
@@ -5150,6 +5152,8 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
if (keep_rel)
{
+ List *fdwPrivList = (List *) list_nth(node->fdwPrivLists, i);
+
resultRelations = lappend_int(resultRelations, rti);
if (node->withCheckOptionLists)
{
@@ -5185,6 +5189,19 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mergeJoinConditions = lappend(mergeJoinConditions, mergeJoinCondition);
}
+
+ /*
+ * fdwPrivLists/fdwDirectModifyPlans are re-indexed to match
+ * resultRelations
+ */
+ fdwPrivLists = lappend(fdwPrivLists, fdwPrivList);
+ if (bms_is_member(i, node->fdwDirectModifyPlans))
+ {
+ int new_index = list_length(resultRelations) - 1;
+
+ fdwDirectModifyPlans = bms_add_member(fdwDirectModifyPlans,
+ new_index);
+ }
}
i++;
}
@@ -5213,6 +5230,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
mtstate->mt_updateColnosLists = updateColnosLists;
mtstate->mt_mergeActionLists = mergeActionLists;
mtstate->mt_mergeJoinConditions = mergeJoinConditions;
+ mtstate->mt_fdwPrivLists = fdwPrivLists;
/*----------
* Resolve the target relation. This is the same as:
@@ -5288,7 +5306,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
/* Initialize the usesFdwDirectModify flag */
resultRelInfo->ri_usesFdwDirectModify =
- bms_is_member(i, node->fdwDirectModifyPlans);
+ bms_is_member(i, fdwDirectModifyPlans);
/*
* Verify result relation is a valid target for the current operation
@@ -5317,7 +5335,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
resultRelInfo->ri_FdwRoutine != NULL &&
resultRelInfo->ri_FdwRoutine->BeginForeignModify != NULL)
{
- List *fdw_private = (List *) list_nth(node->fdwPrivLists, i);
+ List *fdw_private = (List *) list_nth(fdwPrivLists, i);
resultRelInfo->ri_FdwRoutine->BeginForeignModify(mtstate,
resultRelInfo,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 53c138310db..5871383961f 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -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;
+
/*
* Target relation mentioned in the original statement, used to fire
* statement-level triggers and as the root for tuple routing. (This
view thread (32+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19484: Segmentation fault triggered by FDW
In-Reply-To: <CAPmGK15nR_pePb9QAMViq+AGuWeJSqPnLTJVLsr96aU2TFmcmw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox