Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 5c47049

Browse filesBrowse files
author
Etsuro Fujita
committed
postgres_fdw: Fix incorrect handling of row movement for remote partitions.
Commit 3d956d9 added support for update row movement in postgres_fdw. This patch fixes the following issues introduced by that commit: * When a remote partition chosen to insert routed rows into was also an UPDATE subplan target rel that would be updated later, the UPDATE that used a direct modification plan modified those routed rows incorrectly because those routed rows were visible to the later UPDATE command. The right fix for this would be to have some way in postgres_fdw in which the later UPDATE command ignores those routed rows, but it seems hard to do so with the current infrastructure. For now throw an error in that case. * When a remote partition chosen to insert routed rows into was also an UPDATE subplan target rel, fmstate created for the UPDATE that used a non-direct modification plan was mistakenly overridden by another fmstate created for inserting those routed rows into the partition. This caused 1) server crash when the partition would be updated later, and 2) resource leak when the partition had been already updated. To avoid that, adjust the treatment of the fmstate for the inserting. As for #1, since we would also have the incorrectness issue as mentioned above, error out in that case as well. Update the docs to mention that postgres_fdw currently does not handle the case where a remote partition chosen to insert a routed row into is also an UPDATE subplan target rel that will be updated later. Author: Amit Langote and Etsuro Fujita Reviewed-by: Amit Langote Backpatch-through: 11 where row movement in postgres_fdw was added Discussion: https://postgr.es/m/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp
1 parent fdc7efc commit 5c47049
Copy full SHA for 5c47049

File tree

4 files changed

+239
-2
lines changed
Filter options

4 files changed

+239
-2
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/expected/postgres_fdw.out
+131Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7734,6 +7734,137 @@ update utrtest set a = 1 where a = 2 returning *;
77347734
(1 row)
77357735

77367736
drop trigger loct_br_insert_trigger on loct;
7737+
-- We can move rows to a foreign partition that has been updated already,
7738+
-- but can't move rows to a foreign partition that hasn't been updated yet
7739+
delete from utrtest;
7740+
insert into utrtest values (1, 'foo');
7741+
insert into utrtest values (2, 'qux');
7742+
-- Test the former case:
7743+
-- with a direct modification plan
7744+
explain (verbose, costs off)
7745+
update utrtest set a = 1 returning *;
7746+
QUERY PLAN
7747+
-----------------------------------------------------------------
7748+
Update on public.utrtest
7749+
Output: remp.a, remp.b
7750+
Foreign Update on public.remp
7751+
Update on public.locp
7752+
-> Foreign Update on public.remp
7753+
Remote SQL: UPDATE public.loct SET a = 1 RETURNING a, b
7754+
-> Seq Scan on public.locp
7755+
Output: 1, locp.b, locp.ctid
7756+
(8 rows)
7757+
7758+
update utrtest set a = 1 returning *;
7759+
a | b
7760+
---+-----
7761+
1 | foo
7762+
1 | qux
7763+
(2 rows)
7764+
7765+
delete from utrtest;
7766+
insert into utrtest values (1, 'foo');
7767+
insert into utrtest values (2, 'qux');
7768+
-- with a non-direct modification plan
7769+
explain (verbose, costs off)
7770+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
7771+
QUERY PLAN
7772+
------------------------------------------------------------------------------
7773+
Update on public.utrtest
7774+
Output: remp.a, remp.b, "*VALUES*".column1
7775+
Foreign Update on public.remp
7776+
Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1 RETURNING a, b
7777+
Update on public.locp
7778+
-> Hash Join
7779+
Output: 1, remp.b, remp.ctid, "*VALUES*".*, "*VALUES*".column1
7780+
Hash Cond: (remp.a = "*VALUES*".column1)
7781+
-> Foreign Scan on public.remp
7782+
Output: remp.b, remp.ctid, remp.a
7783+
Remote SQL: SELECT a, b, ctid FROM public.loct FOR UPDATE
7784+
-> Hash
7785+
Output: "*VALUES*".*, "*VALUES*".column1
7786+
-> Values Scan on "*VALUES*"
7787+
Output: "*VALUES*".*, "*VALUES*".column1
7788+
-> Hash Join
7789+
Output: 1, locp.b, locp.ctid, "*VALUES*".*, "*VALUES*".column1
7790+
Hash Cond: (locp.a = "*VALUES*".column1)
7791+
-> Seq Scan on public.locp
7792+
Output: locp.b, locp.ctid, locp.a
7793+
-> Hash
7794+
Output: "*VALUES*".*, "*VALUES*".column1
7795+
-> Values Scan on "*VALUES*"
7796+
Output: "*VALUES*".*, "*VALUES*".column1
7797+
(24 rows)
7798+
7799+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
7800+
a | b | x
7801+
---+-----+---
7802+
1 | foo | 1
7803+
1 | qux | 2
7804+
(2 rows)
7805+
7806+
-- Change the definition of utrtest so that the foreign partition get updated
7807+
-- after the local partition
7808+
delete from utrtest;
7809+
alter table utrtest detach partition remp;
7810+
drop foreign table remp;
7811+
alter table loct drop constraint loct_a_check;
7812+
alter table loct add check (a in (3));
7813+
create foreign table remp (a int check (a in (3)), b text) server loopback options (table_name 'loct');
7814+
alter table utrtest attach partition remp for values in (3);
7815+
insert into utrtest values (2, 'qux');
7816+
insert into utrtest values (3, 'xyzzy');
7817+
-- Test the latter case:
7818+
-- with a direct modification plan
7819+
explain (verbose, costs off)
7820+
update utrtest set a = 3 returning *;
7821+
QUERY PLAN
7822+
-----------------------------------------------------------------
7823+
Update on public.utrtest
7824+
Output: locp.a, locp.b
7825+
Update on public.locp
7826+
Foreign Update on public.remp
7827+
-> Seq Scan on public.locp
7828+
Output: 3, locp.b, locp.ctid
7829+
-> Foreign Update on public.remp
7830+
Remote SQL: UPDATE public.loct SET a = 3 RETURNING a, b
7831+
(8 rows)
7832+
7833+
update utrtest set a = 3 returning *; -- ERROR
7834+
ERROR: cannot route tuples into foreign table to be updated "remp"
7835+
-- with a non-direct modification plan
7836+
explain (verbose, costs off)
7837+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *;
7838+
QUERY PLAN
7839+
------------------------------------------------------------------------------
7840+
Update on public.utrtest
7841+
Output: locp.a, locp.b, "*VALUES*".column1
7842+
Update on public.locp
7843+
Foreign Update on public.remp
7844+
Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1 RETURNING a, b
7845+
-> Hash Join
7846+
Output: 3, locp.b, locp.ctid, "*VALUES*".*, "*VALUES*".column1
7847+
Hash Cond: (locp.a = "*VALUES*".column1)
7848+
-> Seq Scan on public.locp
7849+
Output: locp.b, locp.ctid, locp.a
7850+
-> Hash
7851+
Output: "*VALUES*".*, "*VALUES*".column1
7852+
-> Values Scan on "*VALUES*"
7853+
Output: "*VALUES*".*, "*VALUES*".column1
7854+
-> Hash Join
7855+
Output: 3, remp.b, remp.ctid, "*VALUES*".*, "*VALUES*".column1
7856+
Hash Cond: (remp.a = "*VALUES*".column1)
7857+
-> Foreign Scan on public.remp
7858+
Output: remp.b, remp.ctid, remp.a
7859+
Remote SQL: SELECT a, b, ctid FROM public.loct FOR UPDATE
7860+
-> Hash
7861+
Output: "*VALUES*".*, "*VALUES*".column1
7862+
-> Values Scan on "*VALUES*"
7863+
Output: "*VALUES*".*, "*VALUES*".column1
7864+
(24 rows)
7865+
7866+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; -- ERROR
7867+
ERROR: cannot route tuples into foreign table to be updated "remp"
77377868
drop table utrtest;
77387869
drop table loct;
77397870
-- Test copy tuple routing

‎contrib/postgres_fdw/postgres_fdw.c

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+58-2Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,10 @@ typedef struct PgFdwModifyState
185185

186186
/* working memory context */
187187
MemoryContext temp_cxt; /* context for per-tuple temporary data */
188+
189+
/* for update row movement if subplan result rel */
190+
struct PgFdwModifyState *aux_fmstate; /* foreign-insert state, if
191+
* created */
188192
} PgFdwModifyState;
189193

190194
/*
@@ -1844,8 +1848,22 @@ postgresExecForeignInsert(EState *estate,
18441848
TupleTableSlot *slot,
18451849
TupleTableSlot *planSlot)
18461850
{
1847-
return execute_foreign_modify(estate, resultRelInfo, CMD_INSERT,
1851+
PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
1852+
TupleTableSlot *rslot;
1853+
1854+
/*
1855+
* If the fmstate has aux_fmstate set, use the aux_fmstate (see
1856+
* postgresBeginForeignInsert())
1857+
*/
1858+
if (fmstate->aux_fmstate)
1859+
resultRelInfo->ri_FdwState = fmstate->aux_fmstate;
1860+
rslot = execute_foreign_modify(estate, resultRelInfo, CMD_INSERT,
18481861
slot, planSlot);
1862+
/* Revert that change */
1863+
if (fmstate->aux_fmstate)
1864+
resultRelInfo->ri_FdwState = fmstate;
1865+
1866+
return rslot;
18491867
}
18501868

18511869
/*
@@ -1915,6 +1933,22 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
19151933
List *retrieved_attrs = NIL;
19161934
bool doNothing = false;
19171935

1936+
/*
1937+
* If the foreign table we are about to insert routed rows into is also
1938+
* an UPDATE subplan result rel that will be updated later, proceeding
1939+
* with the INSERT will result in the later UPDATE incorrectly modifying
1940+
* those routed rows, so prevent the INSERT --- it would be nice if we
1941+
* could handle this case; but for now, throw an error for safety.
1942+
*/
1943+
if (plan && plan->operation == CMD_UPDATE &&
1944+
(resultRelInfo->ri_usesFdwDirectModify ||
1945+
resultRelInfo->ri_FdwState) &&
1946+
resultRelInfo > mtstate->resultRelInfo + mtstate->mt_whichplan)
1947+
ereport(ERROR,
1948+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
1949+
errmsg("cannot route tuples into foreign table to be updated \"%s\"",
1950+
RelationGetRelationName(rel))));
1951+
19181952
initStringInfo(&sql);
19191953

19201954
/* We transmit all columns that are defined in the foreign table. */
@@ -1983,7 +2017,19 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
19832017
retrieved_attrs != NIL,
19842018
retrieved_attrs);
19852019

1986-
resultRelInfo->ri_FdwState = fmstate;
2020+
/*
2021+
* If the given resultRelInfo already has PgFdwModifyState set, it means
2022+
* the foreign table is an UPDATE subplan result rel; in which case, store
2023+
* the resulting state into the aux_fmstate of the PgFdwModifyState.
2024+
*/
2025+
if (resultRelInfo->ri_FdwState)
2026+
{
2027+
Assert(plan && plan->operation == CMD_UPDATE);
2028+
Assert(resultRelInfo->ri_usesFdwDirectModify == false);
2029+
((PgFdwModifyState *) resultRelInfo->ri_FdwState)->aux_fmstate = fmstate;
2030+
}
2031+
else
2032+
resultRelInfo->ri_FdwState = fmstate;
19872033
}
19882034

19892035
/*
@@ -1998,6 +2044,13 @@ postgresEndForeignInsert(EState *estate,
19982044

19992045
Assert(fmstate != NULL);
20002046

2047+
/*
2048+
* If the fmstate has aux_fmstate set, get the aux_fmstate (see
2049+
* postgresBeginForeignInsert())
2050+
*/
2051+
if (fmstate->aux_fmstate)
2052+
fmstate = fmstate->aux_fmstate;
2053+
20012054
/* Destroy the execution state */
20022055
finish_foreign_modify(fmstate);
20032056
}
@@ -3482,6 +3535,9 @@ create_foreign_modify(EState *estate,
34823535

34833536
Assert(fmstate->p_nums <= n_params);
34843537

3538+
/* Initialize auxiliary state */
3539+
fmstate->aux_fmstate = NULL;
3540+
34853541
return fmstate;
34863542
}
34873543

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/sql/postgres_fdw.sql
+45Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2015,6 +2015,51 @@ update utrtest set a = 1 where a = 2 returning *;
20152015

20162016
drop trigger loct_br_insert_trigger on loct;
20172017

2018+
-- We can move rows to a foreign partition that has been updated already,
2019+
-- but can't move rows to a foreign partition that hasn't been updated yet
2020+
2021+
delete from utrtest;
2022+
insert into utrtest values (1, 'foo');
2023+
insert into utrtest values (2, 'qux');
2024+
2025+
-- Test the former case:
2026+
-- with a direct modification plan
2027+
explain (verbose, costs off)
2028+
update utrtest set a = 1 returning *;
2029+
update utrtest set a = 1 returning *;
2030+
2031+
delete from utrtest;
2032+
insert into utrtest values (1, 'foo');
2033+
insert into utrtest values (2, 'qux');
2034+
2035+
-- with a non-direct modification plan
2036+
explain (verbose, costs off)
2037+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
2038+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
2039+
2040+
-- Change the definition of utrtest so that the foreign partition get updated
2041+
-- after the local partition
2042+
delete from utrtest;
2043+
alter table utrtest detach partition remp;
2044+
drop foreign table remp;
2045+
alter table loct drop constraint loct_a_check;
2046+
alter table loct add check (a in (3));
2047+
create foreign table remp (a int check (a in (3)), b text) server loopback options (table_name 'loct');
2048+
alter table utrtest attach partition remp for values in (3);
2049+
insert into utrtest values (2, 'qux');
2050+
insert into utrtest values (3, 'xyzzy');
2051+
2052+
-- Test the latter case:
2053+
-- with a direct modification plan
2054+
explain (verbose, costs off)
2055+
update utrtest set a = 3 returning *;
2056+
update utrtest set a = 3 returning *; -- ERROR
2057+
2058+
-- with a non-direct modification plan
2059+
explain (verbose, costs off)
2060+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *;
2061+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; -- ERROR
2062+
20182063
drop table utrtest;
20192064
drop table loct;
20202065

‎doc/src/sgml/postgres-fdw.sgml

Copy file name to clipboardExpand all lines: doc/src/sgml/postgres-fdw.sgml
+5Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@
7474
UPDATE</literal> clause. However, the <literal>ON CONFLICT DO NOTHING</literal>
7575
clause is supported, provided a unique index inference specification
7676
is omitted.
77+
Note also that <filename>postgres_fdw</filename> supports row movement
78+
invoked by <command>UPDATE</command> statements executed on partitioned
79+
tables, but it currently does not handle the case where a remote partition
80+
chosen to insert a moved row into is also an <command>UPDATE</command>
81+
target partition that will be updated later.
7782
</para>
7883

7984
<para>

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.