[(known) BUG] DELETE/UPDATE more than one row in partitioned foreign table
Hi all,
We have been bitten by this old bug recently:
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
I suspect this bug lost attention when the only fixed submitted to the
commitfest has been flagged as "returned with feedback":
https://commitfest.postgresql.org/patch/1819/
Please, find in attachment the old patch forbidding more than one row to be
deleted/updated from postgresExecForeignDelete and postgresExecForeignUpdate. I
just rebased them without modification. I suppose this is much safer than
leaving the FDW destroy arbitrary rows on the remote side based on their sole
ctid.
The original discussion talked about using "WHERE CURRENT OF" with cursors to
update/delete rows but discard it because of performance penalty. As adding
tableoid as a junk clause as been rejected, should we investigate the former?
At least for existing major release?
Or maybe we should just not support foreign table to reference a
remote partitioned table?
I'm afraid other fix suggestions from 2018-2019 couldn't be backported as they
seem to require additional feature in FDW altogether. This might be another
reason this bug has been forgotten.
Regards,
Attachments:
v3-0001-Test-exposing-bug-when-foreign-table-points-to-a-.patchtext/x-patchDownload
From d93f026e74659d3387a0ca1bbd8ae94fb0c240e1 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Tue, 15 Jul 2025 12:45:21 +0200
Subject: [PATCH v3 1/2] Test exposing bug when foreign table points to a
partitioned table
When a foreign table points to a partitioned table or an inheritance
parent on the foreign server, a non-direct DML can affect multiple
rows when only one row is intended to be affected. This
happens because postgres_fdw uses only ctid to identify a row to work
on. Though ctid uniquely identifies a row in a single table, in a
partitioned table or in an inheritance hierarchy, there can be be
multiple rows, in different partitions, with the same ctid. So
DML statement sent to the foreign server by postgres_fdw ends up
affecting more than one rows, only one of which is intended to be
affected.
This commit adds testcases to show the problem. A subsequent commit
would have a fix to the problem.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS%2BOxcQo%3DaBDn1COywmcg%40mail.gmail.com
Rebased by Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
---
.../postgres_fdw/expected/postgres_fdw.out | 125 ++++++++++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 58 ++++++++
2 files changed, 183 insertions(+)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 2185b42bb4f..62019eaa881 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8954,6 +8954,131 @@ drop foreign table remt2;
drop table loct1;
drop table loct2;
drop table parent;
+-- test DML statement on a foreign table pointing to an inheritance hierarchy
+-- on the remote server
+CREATE TABLE a(aa TEXT);
+ALTER TABLE a SET (autovacuum_enabled = 'false');
+CREATE TABLE b() INHERITS(a);
+ALTER TABLE b SET (autovacuum_enabled = 'false');
+INSERT INTO a(aa) VALUES('aaa');
+INSERT INTO b(aa) VALUES('bbb');
+CREATE FOREIGN TABLE fa (aa TEXT) SERVER loopback OPTIONS (table_name 'a');
+SELECT tableoid::regclass, ctid, * FROM fa;
+ tableoid | ctid | aa
+----------+-------+-----
+ fa | (0,1) | aaa
+ fa | (0,1) | bbb
+(2 rows)
+
+-- use random() so that DML statement is not pushed down to the foreign
+-- server
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa = 'aaa';
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------
+ Update on public.fa
+ Remote SQL: UPDATE public.a SET aa = $2 WHERE ctid = $1
+ -> Foreign Scan on public.fa
+ Output: CASE WHEN (random() <= '1'::double precision) THEN 'zzzz'::text ELSE NULL::text END, ctid, fa.*
+ Remote SQL: SELECT aa, ctid FROM public.a WHERE ((aa = 'aaa')) FOR UPDATE
+(5 rows)
+
+UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa = 'aaa';
+SELECT tableoid::regclass, ctid, * FROM fa;
+ tableoid | ctid | aa
+----------+-------+------
+ fa | (0,2) | zzzz
+ fa | (0,1) | bbb
+(2 rows)
+
+-- repopulate tables so that we have rows with same ctid
+TRUNCATE a, b;
+INSERT INTO a(aa) VALUES('aaa');
+INSERT INTO b(aa) VALUES('bbb');
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
+ QUERY PLAN
+---------------------------------------------------------------------------------------------------------------
+ Delete on public.fa
+ Remote SQL: DELETE FROM public.a WHERE ctid = $1
+ -> Foreign Scan on public.fa
+ Output: ctid
+ Filter: (fa.aa = CASE WHEN (random() <= '1'::double precision) THEN 'aaa'::text ELSE 'bbb'::text END)
+ Remote SQL: SELECT aa, ctid FROM public.a FOR UPDATE
+(6 rows)
+
+DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
+SELECT tableoid::regclass, ctid, * FROM fa;
+ tableoid | ctid | aa
+----------+-------+-----
+ fa | (0,1) | bbb
+(1 row)
+
+-- cleanup
+DROP FOREIGN TABLE fa;
+DROP TABLE a CASCADE;
+NOTICE: drop cascades to table b
+-- ===================================================================
+-- test foreign table pointing to a remote partitioned table
+-- ===================================================================
+-- test DML statement on foreign table pointing to a foreign partitioned table
+CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
+CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
+CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
+INSERT INTO plt VALUES (1, 1), (2, 2);
+CREATE FOREIGN TABLE fplt (a int, b int) SERVER loopback OPTIONS (table_name 'plt');
+SELECT tableoid::regclass, ctid, * FROM fplt;
+ tableoid | ctid | a | b
+----------+-------+---+---
+ fplt | (0,1) | 1 | 1
+ fplt | (0,1) | 2 | 2
+(2 rows)
+
+-- use random() so that DML statement is not pushed down to the foreign
+-- server
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
+ QUERY PLAN
+-------------------------------------------------------------------------------------------------
+ Update on public.fplt
+ Remote SQL: UPDATE public.plt SET b = $2 WHERE ctid = $1
+ -> Foreign Scan on public.fplt
+ Output: CASE WHEN (random() <= '1'::double precision) THEN 10 ELSE 20 END, ctid, fplt.*
+ Remote SQL: SELECT a, b, ctid FROM public.plt WHERE ((a = 1)) FOR UPDATE
+(5 rows)
+
+UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
+SELECT tableoid::regclass, ctid, * FROM fplt;
+ tableoid | ctid | a | b
+----------+-------+---+----
+ fplt | (0,2) | 1 | 10
+ fplt | (0,1) | 2 | 2
+(2 rows)
+
+-- repopulate partitioned table so that we have rows with same ctid
+TRUNCATE plt;
+INSERT INTO plt VALUES (1, 1), (2, 2);
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
+ QUERY PLAN
+---------------------------------------------------------------------------------------------
+ Delete on public.fplt
+ Remote SQL: DELETE FROM public.plt WHERE ctid = $1
+ -> Foreign Scan on public.fplt
+ Output: ctid
+ Filter: (fplt.a = CASE WHEN (random() <= '1'::double precision) THEN 1 ELSE 10 END)
+ Remote SQL: SELECT a, ctid FROM public.plt FOR UPDATE
+(6 rows)
+
+DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
+SELECT tableoid::regclass, ctid, * FROM fplt;
+ tableoid | ctid | a | b
+----------+-------+---+---
+ fplt | (0,1) | 2 | 2
+(1 row)
+
+DROP TABLE plt;
+DROP FOREIGN TABLE fplt;
-- ===================================================================
-- test tuple routing for foreign-table partitions
-- ===================================================================
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e534b40de3c..ae3777b7edb 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2494,6 +2494,64 @@ drop table loct1;
drop table loct2;
drop table parent;
+-- test DML statement on a foreign table pointing to an inheritance hierarchy
+-- on the remote server
+CREATE TABLE a(aa TEXT);
+ALTER TABLE a SET (autovacuum_enabled = 'false');
+CREATE TABLE b() INHERITS(a);
+ALTER TABLE b SET (autovacuum_enabled = 'false');
+INSERT INTO a(aa) VALUES('aaa');
+INSERT INTO b(aa) VALUES('bbb');
+CREATE FOREIGN TABLE fa (aa TEXT) SERVER loopback OPTIONS (table_name 'a');
+
+SELECT tableoid::regclass, ctid, * FROM fa;
+-- use random() so that DML statement is not pushed down to the foreign
+-- server
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa = 'aaa';
+UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa = 'aaa';
+SELECT tableoid::regclass, ctid, * FROM fa;
+-- repopulate tables so that we have rows with same ctid
+TRUNCATE a, b;
+INSERT INTO a(aa) VALUES('aaa');
+INSERT INTO b(aa) VALUES('bbb');
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
+DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
+SELECT tableoid::regclass, ctid, * FROM fa;
+
+-- cleanup
+DROP FOREIGN TABLE fa;
+DROP TABLE a CASCADE;
+
+-- ===================================================================
+-- test foreign table pointing to a remote partitioned table
+-- ===================================================================
+
+-- test DML statement on foreign table pointing to a foreign partitioned table
+CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
+CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
+CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
+INSERT INTO plt VALUES (1, 1), (2, 2);
+CREATE FOREIGN TABLE fplt (a int, b int) SERVER loopback OPTIONS (table_name 'plt');
+SELECT tableoid::regclass, ctid, * FROM fplt;
+-- use random() so that DML statement is not pushed down to the foreign
+-- server
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
+UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
+SELECT tableoid::regclass, ctid, * FROM fplt;
+-- repopulate partitioned table so that we have rows with same ctid
+TRUNCATE plt;
+INSERT INTO plt VALUES (1, 1), (2, 2);
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
+DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
+SELECT tableoid::regclass, ctid, * FROM fplt;
+
+DROP TABLE plt;
+DROP FOREIGN TABLE fplt;
+
-- ===================================================================
-- test tuple routing for foreign-table partitions
-- ===================================================================
--
2.50.0
v3-0002-Error-out-if-one-iteration-of-non-direct-DML-affe.patchtext/x-patchDownload
From 20d78db73ecc0ccea34d6cab03e9d882564493f6 Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Fri, 18 Jul 2025 16:52:38 +0200
Subject: [PATCH v3 2/2] Error out if one iteration of non-direct DML affects
more than one row on the foreign server
When a foreign table points to a partitioned table or an inheritance
parent on the foreign server, a non-direct DML can affect multiple
rows when only one row is intended to be affected. This happens
because postgres_fdw uses only ctid to identify a row to work on.
Though ctid uniquely identifies a row in a single table, in a
partitioned table or in an inheritance hierarchy, there can be be
multiple rows, in different partitions, with the same ctid. So a DML
statement sent to the foreign server by postgres_fdw ends up affecting
more than one rows, only one of which is intended to be affected.
In such a case it's good to throw an error instead of corrupting
remote database with unwanted UPDATE/DELETEs. Subsequent commits will
try to fix this situation.
Author: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Author: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Rebased by Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
---
.../postgres_fdw/expected/postgres_fdw.out | 26 ++++++++------
contrib/postgres_fdw/postgres_fdw.c | 36 +++++++++++++++----
2 files changed, 46 insertions(+), 16 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 62019eaa881..b0ef54a2889 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8984,10 +8984,11 @@ UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa
(5 rows)
UPDATE fa SET aa = (CASE WHEN random() <= 1 THEN 'zzzz' ELSE NULL END) WHERE aa = 'aaa';
+ERROR: foreign server affected 2 rows when only one was expected
SELECT tableoid::regclass, ctid, * FROM fa;
- tableoid | ctid | aa
-----------+-------+------
- fa | (0,2) | zzzz
+ tableoid | ctid | aa
+----------+-------+-----
+ fa | (0,1) | aaa
fa | (0,1) | bbb
(2 rows)
@@ -9008,11 +9009,13 @@ DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
(6 rows)
DELETE FROM fa WHERE aa = (CASE WHEN random() <= 1 THEN 'aaa' ELSE 'bbb' END);
+ERROR: foreign server affected 2 rows when only one was expected
SELECT tableoid::regclass, ctid, * FROM fa;
- tableoid | ctid | aa
+ tableoid | ctid | aa
----------+-------+-----
+ fa | (0,1) | aaa
fa | (0,1) | bbb
-(1 row)
+(2 rows)
-- cleanup
DROP FOREIGN TABLE fa;
@@ -9048,10 +9051,11 @@ UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
(5 rows)
UPDATE fplt SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 20 END) WHERE a = 1;
+ERROR: foreign server affected 2 rows when only one was expected
SELECT tableoid::regclass, ctid, * FROM fplt;
- tableoid | ctid | a | b
-----------+-------+---+----
- fplt | (0,2) | 1 | 10
+ tableoid | ctid | a | b
+----------+-------+---+---
+ fplt | (0,1) | 1 | 1
fplt | (0,1) | 2 | 2
(2 rows)
@@ -9071,11 +9075,13 @@ DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
(6 rows)
DELETE FROM fplt WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END);
+ERROR: foreign server affected 2 rows when only one was expected
SELECT tableoid::regclass, ctid, * FROM fplt;
tableoid | ctid | a | b
----------+-------+---+---
- fplt | (0,1) | 2 | 2
-(1 row)
+ fplt | (0,1) | 1 | 1
+ fplt | (0,1) | 2 | 2
+(2 rows)
DROP TABLE plt;
DROP FOREIGN TABLE fplt;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e0a34b27c7c..09c87d0e5d8 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4132,7 +4132,8 @@ execute_foreign_modify(EState *estate,
ItemPointer ctid = NULL;
const char **p_values;
PGresult *res;
- int n_rows;
+ int n_rows_returned;
+ int n_rows_affected;
StringInfoData sql;
/* The operation should be INSERT, UPDATE, or DELETE */
@@ -4213,27 +4214,50 @@ execute_foreign_modify(EState *estate,
pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
/* Check number of rows affected, and fetch RETURNING tuple if any */
+ n_rows_affected = atoi(PQcmdTuples(res));
if (fmstate->has_returning)
{
Assert(*numSlots == 1);
- n_rows = PQntuples(res);
- if (n_rows > 0)
+ n_rows_returned = PQntuples(res);
+ if (n_rows_returned > 0)
store_returning_result(fmstate, slots[0], res);
+
+ // FIXME: shouldn't we check the max number of rows returned is one?
}
else
- n_rows = atoi(PQcmdTuples(res));
+ n_rows_returned = 0;
/* And clean up */
PQclear(res);
MemoryContextReset(fmstate->temp_cxt);
- *numSlots = n_rows;
+ /*
+ * UPDATE & DELETE command can only affect one row, make sure this contract
+ * is respected.
+ * CMD_INSERT can insert multiple row when called from ForeignBatchInsert.
+ */
+ if (operation != CMD_INSERT)
+ {
+ /* No rows should be returned if no rows were affected */
+ if (n_rows_affected == 0 && n_rows_returned != 0)
+ elog(ERROR, "foreign server returned %d rows when no row was affected",
+ n_rows_returned);
+
+ /* ERROR if more than one row was updated on the remote end */
+ if (n_rows_affected > 1)
+ ereport(ERROR,
+ (errcode (ERRCODE_FDW_ERROR), /* XXX */
+ errmsg ("foreign server affected %d rows when only one was expected",
+ n_rows_affected)));
+ }
+
+ *numSlots = n_rows_returned;
/*
* Return NULL if nothing was inserted/updated/deleted on the remote end
*/
- return (n_rows > 0) ? slots : NULL;
+ return (n_rows_affected > 0) ? slots : NULL;
}
/*
--
2.50.0
Hi,
On Sat, Jul 19, 2025 at 12:53 AM Jehan-Guillaume de Rorthais
<jgdr@dalibo.com> wrote:
We have been bitten by this old bug recently:
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
I suspect this bug lost attention when the only fixed submitted to the
commitfest has been flagged as "returned with feedback":
This is on my TODO list, but I didn't have time to work on it, unfortunately.
Please, find in attachment the old patch forbidding more than one row to be
deleted/updated from postgresExecForeignDelete and postgresExecForeignUpdate. I
just rebased them without modification. I suppose this is much safer than
leaving the FDW destroy arbitrary rows on the remote side based on their sole
ctid.
Thanks for rebasing the patch set, but I don't think the idea
implemented in the second patch is safe; even with the patch we have:
create table plt (a int, b int) partition by list (a);
create table plt_p1 partition of plt for values in (1);
create table plt_p2 partition of plt for values in (2);
create function trig_null() returns trigger language plpgsql as
$$ begin return null; end; $$;
create trigger trig_null before update or delete on plt_p1
for each row execute function trig_null();
create foreign table fplt (a int, b int)
server loopback options (table_name 'plt');
insert into fplt values (1, 1), (2, 2);
INSERT 0 0
select tableoid::regclass, ctid, * from plt;
tableoid | ctid | a | b
----------+-------+---+---
plt_p1 | (0,1) | 1 | 1
plt_p2 | (0,1) | 2 | 2
(2 rows)
explain verbose update fplt set b = (case when random() <= 1 then 10
else 20 end) where a = 1;
QUERY PLAN
-------------------------------------------------------------------------------------------------
Update on public.fplt (cost=100.00..128.02 rows=0 width=0)
Remote SQL: UPDATE public.plt SET b = $2 WHERE ctid = $1
-> Foreign Scan on public.fplt (cost=100.00..128.02 rows=7 width=42)
Output: CASE WHEN (random() <= '1'::double precision) THEN 10
ELSE 20 END, ctid, fplt.*
Remote SQL: SELECT a, b, ctid FROM public.plt WHERE ((a = 1))
FOR UPDATE
(5 rows)
update fplt set b = (case when random() <= 1 then 10 else 20 end) where a = 1;
UPDATE 1
select tableoid::regclass, ctid, * from plt;
tableoid | ctid | a | b
----------+-------+---+----
plt_p1 | (0,1) | 1 | 1
plt_p2 | (0,2) | 2 | 10
(2 rows)
The row in the partition plt_p2 is updated, which is wrong as the row
doesn't satisfy the query's WHERE condition.
Or maybe we should just not support foreign table to reference a
remote partitioned table?
I don't think so because we can execute SELECT, INSERT, and direct
UPDATE/DELETE safely on such a foreign table.
I think a simple fix for this is to teach the system that the foreign
table is a partitioned table; in more detail, I would like to propose
to 1) add to postgres_fdw a table option, inherited, to indicate
whether the foreign table is a partitioned/inherited table or not, and
2) modify postgresPlanForeignModify() to throw an error if the given
operation is an update/delete on such a foreign table. Attached is a
WIP patch for that. I think it is the user's responsibility to set
the option properly, but we could modify postgresImportForeignSchema()
to support that. Also, I think this would be back-patchable.
What do you think about that?
Best regards,
Etsuro Fujita
Attachments:
postgres_fdw-disallow-upddel-in-problematic-cases-wip.patchapplication/octet-stream; name=postgres_fdw-disallow-upddel-in-problematic-cases-wip.patchDownload
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index fa9ee716067..fdadfb06ef9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7121,6 +7121,33 @@ RESET enable_material;
DROP FOREIGN TABLE remt2;
DROP TABLE loct1;
DROP TABLE loct2;
+-- Test non-pushed-down UPDATE/DELETE on partitioned/inherited foreign tables
+CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
+CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
+CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
+CREATE FOREIGN TABLE inh_ft (a int, b int) SERVER loopback OPTIONS (table_name 'plt', inherited 'true');
+INSERT INTO inh_ft VALUES (1, 1), (2, 2);
+SELECT tableoid::regclass, ctid, * FROM inh_ft;
+ tableoid | ctid | a | b
+----------+-------+---+---
+ inh_ft | (0,1) | 1 | 1
+ inh_ft | (0,1) | 2 | 2
+(2 rows)
+
+-- Use random() so that UPDATE/DELETE is not pushed down to the remote
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE inh_ft SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 100 END) WHERE a = 1; -- should fail
+ERROR: cannot update foreign table "inh_ft"
+UPDATE inh_ft SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 100 END) WHERE a = 1; -- should fail
+ERROR: cannot update foreign table "inh_ft"
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM inh_ft WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END); -- should fail
+ERROR: cannot delete from foreign table "inh_ft"
+DELETE FROM inh_ft WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END); -- should fail
+ERROR: cannot delete from foreign table "inh_ft"
+-- Cleanup
+DROP FOREIGN TABLE inh_ft;
+DROP TABLE plt;
-- ===================================================================
-- test check constraints
-- ===================================================================
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index c2f936640bc..85b40b948c3 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -119,7 +119,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
/*
* Validate option value, when we can do so without any context.
*/
- if (strcmp(def->defname, "use_remote_estimate") == 0 ||
+ if (strcmp(def->defname, "inherited") == 0 ||
+ strcmp(def->defname, "use_remote_estimate") == 0 ||
strcmp(def->defname, "updatable") == 0 ||
strcmp(def->defname, "truncatable") == 0 ||
strcmp(def->defname, "async_capable") == 0 ||
@@ -247,6 +248,7 @@ InitPgFdwOptions(void)
{"schema_name", ForeignTableRelationId, false},
{"table_name", ForeignTableRelationId, false},
{"column_name", AttributeRelationId, false},
+ {"inherited", ForeignTableRelationId, false},
/* use_remote_estimate is available on both server and table */
{"use_remote_estimate", ForeignServerRelationId, false},
{"use_remote_estimate", ForeignTableRelationId, false},
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e0a34b27c7c..e5d186dc62c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -649,6 +649,7 @@ postgresGetForeignRelSize(PlannerInfo *root,
* use_remote_estimate, fetch_size and async_capable override per-server
* settings of them, respectively.
*/
+ fpinfo->inherited = false;
fpinfo->use_remote_estimate = false;
fpinfo->fdw_startup_cost = DEFAULT_FDW_STARTUP_COST;
fpinfo->fdw_tuple_cost = DEFAULT_FDW_TUPLE_COST;
@@ -1794,6 +1795,28 @@ postgresPlanForeignModify(PlannerInfo *root,
bool doNothing = false;
int values_end_len = -1;
+ /*
+ * We don't currently support update/delete on partitioned/inherited
+ * foreign tables via ExecForeignUpdate()/ExecForeignDelete(); if the
+ * operation is update/delete, check whether the foreign table is
+ * partitioned/inherited or not, and if so, throw an error. Note that
+ * we would have already determined that the operation on the foreign
+ * table cannot be executed via DirectModify before we get here.
+ */
+ if (operation == CMD_UPDATE || operation == CMD_DELETE)
+ {
+ RelOptInfo *rel = find_base_rel(root, resultRelation);
+ PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) rel->fdw_private;
+
+ if (fpinfo && fpinfo->inherited)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg((operation == CMD_UPDATE) ?
+ "cannot update foreign table \"%s\"" :
+ "cannot delete from foreign table \"%s\"",
+ get_rel_name(rte->relid))));
+ }
+
initStringInfo(&sql);
/*
@@ -6292,7 +6315,9 @@ apply_table_options(PgFdwRelationInfo *fpinfo)
{
DefElem *def = (DefElem *) lfirst(lc);
- if (strcmp(def->defname, "use_remote_estimate") == 0)
+ if (strcmp(def->defname, "inherited") == 0)
+ fpinfo->inherited = defGetBoolean(def);
+ else if (strcmp(def->defname, "use_remote_estimate") == 0)
fpinfo->use_remote_estimate = defGetBoolean(def);
else if (strcmp(def->defname, "fetch_size") == 0)
(void) parse_int(defGetString(def), &fpinfo->fetch_size, 0, NULL);
@@ -6414,6 +6439,8 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
joinrel->fdw_private = fpinfo;
/* attrs_used is only for base relations. */
fpinfo->attrs_used = NULL;
+ /* inherited is only for base relations. */
+ fpinfo->inherited = false;
/*
* If there is a possibility that EvalPlanQual will be executed, we need
@@ -6780,6 +6807,8 @@ postgresGetForeignUpperPaths(PlannerInfo *root, UpperRelationKind stage,
fpinfo = (PgFdwRelationInfo *) palloc0(sizeof(PgFdwRelationInfo));
fpinfo->pushdown_safe = false;
+ /* inherited is only for base relations. */
+ fpinfo->inherited = false;
fpinfo->stage = stage;
output_rel->fdw_private = fpinfo;
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 81358f3bde7..7163b2966dd 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -76,10 +76,12 @@ typedef struct PgFdwRelationInfo
Cost rel_total_cost;
/* Options extracted from catalogs. */
+ bool inherited;
bool use_remote_estimate;
Cost fdw_startup_cost;
Cost fdw_tuple_cost;
List *shippable_extensions; /* OIDs of shippable extensions */
+ int fetch_size; /* fetch size for this remote table */
bool async_capable;
/* Cached catalog information. */
@@ -87,8 +89,6 @@ typedef struct PgFdwRelationInfo
ForeignServer *server;
UserMapping *user; /* only set in use_remote_estimate mode */
- int fetch_size; /* fetch size for this remote table */
-
/*
* Name of the relation, for use while EXPLAINing ForeignScan. It is used
* for join and upper relations but is set for all relations. For a base
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index b449e122abe..f447eba29dd 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1725,6 +1725,28 @@ DROP FOREIGN TABLE remt2;
DROP TABLE loct1;
DROP TABLE loct2;
+-- Test non-pushed-down UPDATE/DELETE on partitioned/inherited foreign tables
+CREATE TABLE plt (a int, b int) PARTITION BY LIST(a);
+CREATE TABLE plt_p1 PARTITION OF plt FOR VALUES IN (1);
+CREATE TABLE plt_p2 PARTITION OF plt FOR VALUES IN (2);
+CREATE FOREIGN TABLE inh_ft (a int, b int) SERVER loopback OPTIONS (table_name 'plt', inherited 'true');
+
+INSERT INTO inh_ft VALUES (1, 1), (2, 2);
+SELECT tableoid::regclass, ctid, * FROM inh_ft;
+
+-- Use random() so that UPDATE/DELETE is not pushed down to the remote
+EXPLAIN (VERBOSE, COSTS OFF)
+UPDATE inh_ft SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 100 END) WHERE a = 1; -- should fail
+UPDATE inh_ft SET b = (CASE WHEN random() <= 1 THEN 10 ELSE 100 END) WHERE a = 1; -- should fail
+
+EXPLAIN (VERBOSE, COSTS OFF)
+DELETE FROM inh_ft WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END); -- should fail
+DELETE FROM inh_ft WHERE a = (CASE WHEN random() <= 1 THEN 1 ELSE 10 END); -- should fail
+
+-- Cleanup
+DROP FOREIGN TABLE inh_ft;
+DROP TABLE plt;
+
-- ===================================================================
-- test check constraints
-- ===================================================================
On Wed, Jul 23, 2025 at 7:38 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
I think a simple fix for this is to teach the system that the foreign
table is a partitioned table; in more detail, I would like to propose
to 1) add to postgres_fdw a table option, inherited, to indicate
whether the foreign table is a partitioned/inherited table or not, and
2) modify postgresPlanForeignModify() to throw an error if the given
operation is an update/delete on such a foreign table. Attached is a
WIP patch for that. I think it is the user's responsibility to set
the option properly, but we could modify postgresImportForeignSchema()
to support that. Also, I think this would be back-patchable.
I noticed that this issue occurs in other cases: eg, if a foreign
table is an updatable view on a partitioned/inherited table,
non-pushed-down updates/deletes on the foreign table have the same
issue. So adding the support in postgresImportForeignSchema() is not
that simple. I feel like leaving it to the user, at least for
back-branches.
Best regards,
Etsuro Fujita
Hi,
On Wed, 23 Jul 2025 19:38:19 +0900
Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
Hi,
On Sat, Jul 19, 2025 at 12:53 AM Jehan-Guillaume de Rorthais
<jgdr@dalibo.com> wrote:[…]
Please, find in attachment the old patch forbidding more than one row to be
deleted/updated from postgresExecForeignDelete and
postgresExecForeignUpdate. I just rebased them without modification. I
suppose this is much safer than leaving the FDW destroy arbitrary rows on
the remote side based on their sole ctid.Thanks for rebasing the patch set, but I don't think the idea
implemented in the second patch is safe; even with the patch we have:[…]
The row in the partition plt_p2 is updated, which is wrong as the row
doesn't satisfy the query's WHERE condition.
That's a clever test to expose the weakness of this patch…
Or maybe we should just not support foreign table to reference a
remote partitioned table?I don't think so because we can execute SELECT, INSERT, and direct
UPDATE/DELETE safely on such a foreign table.
Sure, but it's still possible to create one local foreign partition pointing to
remote foreign equivalent. And it seems safer considering how hard it seems to
keep corruptions away from the current situation.
I think a simple fix for this is to teach the system that the foreign
table is a partitioned table; in more detail, I would like to propose
to 1) add to postgres_fdw a table option, inherited, to indicate
whether the foreign table is a partitioned/inherited table or not, and
2) modify postgresPlanForeignModify() to throw an error if the given
operation is an update/delete on such a foreign table. Attached is a
WIP patch for that. I think it is the user's responsibility to set
the option properly, but we could modify postgresImportForeignSchema()
to support that. Also, I think this would be back-patchable.
So it's just a flag the user must set to allow/disallow UPDATE/DELETE on a
foreign table. I'm not convinced by this solution as users can still
easily corrupt their data just because they overlooked the documentation.
What about the first solution Ashutosh Bapat was suggesting: «Use WHERE CURRENT
OF with cursors to update rows.» ?
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.com
It seems to me it never has been explored, is it?
Regards,
On Wed, Jul 30, 2025 at 12:48 AM Jehan-Guillaume de Rorthais
<jgdr@dalibo.com> wrote:
On Wed, 23 Jul 2025 19:38:19 +0900
Etsuro Fujita <etsuro.fujita@gmail.com> wrote:On Sat, Jul 19, 2025 at 12:53 AM Jehan-Guillaume de Rorthais
<jgdr@dalibo.com> wrote:Or maybe we should just not support foreign table to reference a
remote partitioned table?I don't think so because we can execute SELECT, INSERT, and direct
UPDATE/DELETE safely on such a foreign table.Sure, but it's still possible to create one local foreign partition pointing to
remote foreign equivalent. And it seems safer considering how hard it seems to
keep corruptions away from the current situation.
Yeah, that would be a simple workaround for this issue.
I think a simple fix for this is to teach the system that the foreign
table is a partitioned table; in more detail, I would like to propose
to 1) add to postgres_fdw a table option, inherited, to indicate
whether the foreign table is a partitioned/inherited table or not, and
2) modify postgresPlanForeignModify() to throw an error if the given
operation is an update/delete on such a foreign table. Attached is a
WIP patch for that. I think it is the user's responsibility to set
the option properly, but we could modify postgresImportForeignSchema()
to support that. Also, I think this would be back-patchable.So it's just a flag the user must set to allow/disallow UPDATE/DELETE on a
foreign table. I'm not convinced by this solution as users can still
easily corrupt their data just because they overlooked the documentation.What about the first solution Ashutosh Bapat was suggesting: «Use WHERE CURRENT
OF with cursors to update rows.» ?
/messages/by-id/CAFjFpRfcgwsHRmpvoOK-GUQi-n8MgAS+OxcQo=aBDn1COywmcg@mail.gmail.comIt seems to me it never has been explored, is it?
My concern about that solution is: as mentioned by him, it requires
fetching only one row from the remote at a time, which would lead to
large performance degradation when updating many rows.
Best regards,
Etsuro Fujita