[(known) BUG] DELETE/UPDATE more than one row in partitioned foreign table

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":

https://commitfest.postgresql.org/patch/1819/

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

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

It 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

Hi hackers!

We've been bitten by this bug recently too. So I've taken off from the
previous approach,
have fixed some issues and developed it a but further, please check this
POC patch.

While working on it I've stumbled upon a bunch of checks inside copy
slot/tuple machinery
which considered only case when source and destination match, with an old
commentary
that this is not very good and has to be improved, so I've slightly
modified this functionality
for my purposes.

On Wed, Aug 6, 2025 at 2:25 PM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

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

It 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

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/