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

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 e899df6 commit 7a3d055
Copy full SHA for 7a3d055

File tree

4 files changed

+231
-1
lines changed
Filter options

4 files changed

+231
-1
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
@@ -7753,6 +7753,137 @@ update utrtest set a = 1 where a = 2 returning *;
77537753
(1 row)
77547754

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

‎contrib/postgres_fdw/postgres_fdw.c

Copy file name to clipboardExpand all lines: contrib/postgres_fdw/postgres_fdw.c
+50-1Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,10 @@ typedef struct PgFdwModifyState
183183

184184
/* working memory context */
185185
MemoryContext temp_cxt; /* context for per-tuple temporary data */
186+
187+
/* for update row movement if subplan result rel */
188+
struct PgFdwModifyState *aux_fmstate; /* foreign-insert state, if
189+
* created */
186190
} PgFdwModifyState;
187191

188192
/*
@@ -1773,6 +1777,13 @@ postgresExecForeignInsert(EState *estate,
17731777
PGresult *res;
17741778
int n_rows;
17751779

1780+
/*
1781+
* If the fmstate has aux_fmstate set, use the aux_fmstate (see
1782+
* postgresBeginForeignInsert())
1783+
*/
1784+
if (fmstate->aux_fmstate)
1785+
fmstate = fmstate->aux_fmstate;
1786+
17761787
/* Set up the prepared statement on the remote server, if we didn't yet */
17771788
if (!fmstate->p_name)
17781789
prepare_foreign_modify(fmstate);
@@ -2013,6 +2024,22 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
20132024
List *retrieved_attrs = NIL;
20142025
bool doNothing = false;
20152026

2027+
/*
2028+
* If the foreign table we are about to insert routed rows into is also
2029+
* an UPDATE subplan result rel that will be updated later, proceeding
2030+
* with the INSERT will result in the later UPDATE incorrectly modifying
2031+
* those routed rows, so prevent the INSERT --- it would be nice if we
2032+
* could handle this case; but for now, throw an error for safety.
2033+
*/
2034+
if (plan && plan->operation == CMD_UPDATE &&
2035+
(resultRelInfo->ri_usesFdwDirectModify ||
2036+
resultRelInfo->ri_FdwState) &&
2037+
resultRelInfo > mtstate->resultRelInfo + mtstate->mt_whichplan)
2038+
ereport(ERROR,
2039+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
2040+
errmsg("cannot route tuples into foreign table to be updated \"%s\"",
2041+
RelationGetRelationName(rel))));
2042+
20162043
initStringInfo(&sql);
20172044

20182045
/* We transmit all columns that are defined in the foreign table. */
@@ -2079,7 +2106,19 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
20792106
retrieved_attrs != NIL,
20802107
retrieved_attrs);
20812108

2082-
resultRelInfo->ri_FdwState = fmstate;
2109+
/*
2110+
* If the given resultRelInfo already has PgFdwModifyState set, it means
2111+
* the foreign table is an UPDATE subplan result rel; in which case, store
2112+
* the resulting state into the aux_fmstate of the PgFdwModifyState.
2113+
*/
2114+
if (resultRelInfo->ri_FdwState)
2115+
{
2116+
Assert(plan && plan->operation == CMD_UPDATE);
2117+
Assert(resultRelInfo->ri_usesFdwDirectModify == false);
2118+
((PgFdwModifyState *) resultRelInfo->ri_FdwState)->aux_fmstate = fmstate;
2119+
}
2120+
else
2121+
resultRelInfo->ri_FdwState = fmstate;
20832122
}
20842123

20852124
/*
@@ -2094,6 +2133,13 @@ postgresEndForeignInsert(EState *estate,
20942133

20952134
Assert(fmstate != NULL);
20962135

2136+
/*
2137+
* If the fmstate has aux_fmstate set, get the aux_fmstate (see
2138+
* postgresBeginForeignInsert())
2139+
*/
2140+
if (fmstate->aux_fmstate)
2141+
fmstate = fmstate->aux_fmstate;
2142+
20972143
/* Destroy the execution state */
20982144
finish_foreign_modify(fmstate);
20992145
}
@@ -3390,6 +3436,9 @@ create_foreign_modify(EState *estate,
33903436

33913437
Assert(fmstate->p_nums <= n_params);
33923438

3439+
/* Initialize auxiliary state */
3440+
fmstate->aux_fmstate = NULL;
3441+
33933442
return fmstate;
33943443
}
33953444

‎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
@@ -1943,6 +1943,51 @@ update utrtest set a = 1 where a = 2 returning *;
19431943

19441944
drop trigger loct_br_insert_trigger on loct;
19451945

1946+
-- We can move rows to a foreign partition that has been updated already,
1947+
-- but can't move rows to a foreign partition that hasn't been updated yet
1948+
1949+
delete from utrtest;
1950+
insert into utrtest values (1, 'foo');
1951+
insert into utrtest values (2, 'qux');
1952+
1953+
-- Test the former case:
1954+
-- with a direct modification plan
1955+
explain (verbose, costs off)
1956+
update utrtest set a = 1 returning *;
1957+
update utrtest set a = 1 returning *;
1958+
1959+
delete from utrtest;
1960+
insert into utrtest values (1, 'foo');
1961+
insert into utrtest values (2, 'qux');
1962+
1963+
-- with a non-direct modification plan
1964+
explain (verbose, costs off)
1965+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
1966+
update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
1967+
1968+
-- Change the definition of utrtest so that the foreign partition get updated
1969+
-- after the local partition
1970+
delete from utrtest;
1971+
alter table utrtest detach partition remp;
1972+
drop foreign table remp;
1973+
alter table loct drop constraint loct_a_check;
1974+
alter table loct add check (a in (3));
1975+
create foreign table remp (a int check (a in (3)), b text) server loopback options (table_name 'loct');
1976+
alter table utrtest attach partition remp for values in (3);
1977+
insert into utrtest values (2, 'qux');
1978+
insert into utrtest values (3, 'xyzzy');
1979+
1980+
-- Test the latter case:
1981+
-- with a direct modification plan
1982+
explain (verbose, costs off)
1983+
update utrtest set a = 3 returning *;
1984+
update utrtest set a = 3 returning *; -- ERROR
1985+
1986+
-- with a non-direct modification plan
1987+
explain (verbose, costs off)
1988+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *;
1989+
update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; -- ERROR
1990+
19461991
drop table utrtest;
19471992
drop table loct;
19481993

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