(2019/04/19 14:39), Etsuro Fujita wrote:
(2019/04/19 13:00), Amit Langote wrote:
On 2019/04/18 22:10, Etsuro Fujita wrote:
On 2019/04/18 14:06, Amit Langote wrote:
On 2019/04/17 21:49, Etsuro Fujita wrote:
So what I'm thinking is to throw an error for cases like this.
(Though, I
think we should keep to allow rows to be moved from local
partitions to a
foreign-table subplan targetrel that has been updated already.)
Hmm, how would you distinguish (totally inside postgred_fdw I
presume) the
two cases?
One thing I came up with to do that is this:
@@ -1917,6 +1937,23 @@ postgresBeginForeignInsert(ModifyTableState
*mtstate,
initStringInfo(&sql);
+ /*
+ * If the foreign table is an UPDATE subplan resultrel that hasn't
yet
+ * been updated, routing tuples to the table might yield incorrect
+ * results, because if routing tuples, routed tuples will be
mistakenly
+ * read from the table and updated twice when updating the table
--- it
+ * would be nice if we could handle this case; but for now, throw
an error
+ * for safety.
+ */
+ if (plan&& plan->operation == CMD_UPDATE&&
+ (resultRelInfo->ri_usesFdwDirectModify ||
+ resultRelInfo->ri_FdwState)&&
+ resultRelInfo> mtstate->resultRelInfo +
mtstate->mt_whichplan)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot route tuples into foreign
table to be updated \"%s\"",
+ RelationGetRelationName(rel))));
Oh, I see.
So IIUC, you're making postgresBeginForeignInsert() check two things:
1. Whether the foreign table it's about to begin inserting (moved) rows
into is a subplan result rel, checked by
(resultRelInfo->ri_usesFdwDirectModify || resultRelInfo->ri_FdwState)
2. Whether the foreign table it's about to begin inserting (moved) rows
into will be updated later, checked by (resultRelInfo>
mtstate->resultRelInfo + mtstate->mt_whichplan)
This still allows a foreign table to receive rows moved from the local
partitions if it has already finished the UPDATE operation.
Seems reasonable to me.
Forgot to say that since this is a separate issue from the original bug
report, maybe we can first finish fixing the latter. What to do you
think?
Yeah, I think we can do that, but my favorite would be to fix the two
issues in a single shot, because they seem to me rather close to each
other. So I updated a new version in a single patch, which I'm
attaching.
I agree that we can move to fix the other issue right away as the fix you
outlined above seems reasonable, but I wonder if it wouldn't be better to
commit the two fixes separately? The two fixes, although small, are
somewhat complicated and combining them in a single commit might be
confusing. Also, a combined commit might make it harder for the release
note author to list down the exact set of problems being fixed.
OK, I'll separate the patch into two.
After I tried to separate the patch a bit, I changed my mind: I think we
should commit the two issues in a single patch, because the original
issue that overriding fmstate for the UPDATE operation mistakenly by
fmstate for the INSERT operation caused backend crash is fixed by what I
proposed above. So I add the commit message to the previous single
patch, as you suggested.
Some mostly cosmetic comments on the code changes:
* In the following comment:
+ /*
+ * If the foreign table is an UPDATE subplan resultrel that hasn't yet
+ * been updated, routing tuples to the table might yield incorrect
+ * results, because if routing tuples, routed tuples will be mistakenly
+ * read from the table and updated twice when updating the table --- it
+ * would be nice if we could handle this case; but for now, throw an
error
+ * for safety.
+ */
the part that start with "because if routing tuples..." reads a bit
unclear to me. How about writing this as:
/*
* If the foreign table we are about to insert routed rows into is
* also an UPDATE result rel and the UPDATE hasn't been performed yet,
* proceeding with the INSERT will result in the later UPDATE
* incorrectly modifying those routed rows, so prevent the INSERT ---
* it would be nice if we could handle this case, but for now, throw
* an error for safety.
*/
I think that's better than mine; will use that wording.
Done. I simplified your wording a little bit, though.
I see that in all the hunks involving some manipulation of aux_fmstate,
there's a comment explaining what it is, which seems a bit repetitive. I
can see more or less the same explanation in postgresExecForeignInsert(),
postgresBeginForeignInsert(), and postgresEndForeignInsert(). Maybe just
keep the description in postgresBeginForeignInsert as follows:
@@ -1983,7 +2020,19 @@ postgresBeginForeignInsert(ModifyTableState
*mtstate,
retrieved_attrs != NIL,
retrieved_attrs);
- resultRelInfo->ri_FdwState = fmstate;
+ /*
+ * If the given resultRelInfo already has PgFdwModifyState set, it means
+ * the foreign table is an UPDATE subplan resultrel; in which case,
store
+ * the resulting state into the aux_fmstate of the PgFdwModifyState.
+ */
and change the other sites to refer to postgresBeginForeingInsert for the
detailed explanation of what aux_fmstate is.
Good idea; will do.
Done.
Other changes:
* I updated the docs in postgres-fdw.sgml to mention the limitation.
* I did some cleanups for the regression tests.
Please find attached an updated version of the patch.
Best regards,
Etsuro Fujita
>From 77c7d247bbc26a99e6fa2792152cc28dc327c946 Mon Sep 17 00:00:00 2001
From: Etsuro Fujita <efuj...@postgresql.org>
Date: Mon, 22 Apr 2019 19:55:26 +0900
Subject: [PATCH] postgres_fdw: Fix incorrect handling of row movement for
remote partitions.
Commit 3d956d9562 added support for update row movement in postgres_fdw,
and allowed it to insert rows moved from local partitions into a remote
partition. This patch fixes the following issues introduced by the that
commit:
* When a remote partition chosen to insert rouoted rows into was also an
UPDATE subplan target rel that would be updated later, the UPDATE that
uses a direct modification plan modified those routed rows incorrectly
because those routed rows were visible by 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 uses 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 the support for row movement was added
Discussion: https://postgr.es/m/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce...@lab.ntt.co.jp
---
.../postgres_fdw/expected/postgres_fdw.out | 131 ++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 60 +++++++-
contrib/postgres_fdw/sql/postgres_fdw.sql | 45 ++++++
doc/src/sgml/postgres-fdw.sgml | 5 +
4 files changed, 239 insertions(+), 2 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 3ed52709ee..48ea67aaec 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7734,6 +7734,137 @@ update utrtest set a = 1 where a = 2 returning *;
(1 row)
drop trigger loct_br_insert_trigger on loct;
+-- We can move rows to a foreign partition that has been updated already,
+-- but can't move rows to a foreign partition that hasn't been updated yet
+delete from utrtest;
+insert into utrtest values (1, 'foo');
+insert into utrtest values (2, 'qux');
+-- Test the former case:
+-- with a direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 1 returning *;
+ QUERY PLAN
+-----------------------------------------------------------------
+ Update on public.utrtest
+ Output: remp.a, remp.b
+ Foreign Update on public.remp
+ Update on public.locp
+ -> Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = 1 RETURNING a, b
+ -> Seq Scan on public.locp
+ Output: 1, locp.b, locp.ctid
+(8 rows)
+
+update utrtest set a = 1 returning *;
+ a | b
+---+-----
+ 1 | foo
+ 1 | qux
+(2 rows)
+
+delete from utrtest;
+insert into utrtest values (1, 'foo');
+insert into utrtest values (2, 'qux');
+-- with a non-direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
+ QUERY PLAN
+------------------------------------------------------------------------------
+ Update on public.utrtest
+ Output: remp.a, remp.b, "*VALUES*".column1
+ Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1 RETURNING a, b
+ Update on public.locp
+ -> Hash Join
+ Output: 1, remp.b, remp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (remp.a = "*VALUES*".column1)
+ -> Foreign Scan on public.remp
+ Output: remp.b, remp.ctid, remp.a
+ Remote SQL: SELECT a, b, ctid FROM public.loct FOR UPDATE
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Hash Join
+ Output: 1, locp.b, locp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (locp.a = "*VALUES*".column1)
+ -> Seq Scan on public.locp
+ Output: locp.b, locp.ctid, locp.a
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+(24 rows)
+
+update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
+ a | b | x
+---+-----+---
+ 1 | foo | 1
+ 1 | qux | 2
+(2 rows)
+
+-- Change the definition of utrtest so that the foreign partition get updated
+-- after the local partition
+delete from utrtest;
+alter table utrtest detach partition remp;
+drop foreign table remp;
+alter table loct drop constraint loct_a_check;
+alter table loct add check (a in (3));
+create foreign table remp (a int check (a in (3)), b text) server loopback options (table_name 'loct');
+alter table utrtest attach partition remp for values in (3);
+insert into utrtest values (2, 'qux');
+insert into utrtest values (3, 'xyzzy');
+-- Test the latter case:
+-- with a direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 3 returning *;
+ QUERY PLAN
+-----------------------------------------------------------------
+ Update on public.utrtest
+ Output: locp.a, locp.b
+ Update on public.locp
+ Foreign Update on public.remp
+ -> Seq Scan on public.locp
+ Output: 3, locp.b, locp.ctid
+ -> Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = 3 RETURNING a, b
+(8 rows)
+
+update utrtest set a = 3 returning *; -- ERROR
+ERROR: cannot route tuples into foreign table to be updated "remp"
+-- with a non-direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *;
+ QUERY PLAN
+------------------------------------------------------------------------------
+ Update on public.utrtest
+ Output: locp.a, locp.b, "*VALUES*".column1
+ Update on public.locp
+ Foreign Update on public.remp
+ Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1 RETURNING a, b
+ -> Hash Join
+ Output: 3, locp.b, locp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (locp.a = "*VALUES*".column1)
+ -> Seq Scan on public.locp
+ Output: locp.b, locp.ctid, locp.a
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Hash Join
+ Output: 3, remp.b, remp.ctid, "*VALUES*".*, "*VALUES*".column1
+ Hash Cond: (remp.a = "*VALUES*".column1)
+ -> Foreign Scan on public.remp
+ Output: remp.b, remp.ctid, remp.a
+ Remote SQL: SELECT a, b, ctid FROM public.loct FOR UPDATE
+ -> Hash
+ Output: "*VALUES*".*, "*VALUES*".column1
+ -> Values Scan on "*VALUES*"
+ Output: "*VALUES*".*, "*VALUES*".column1
+(24 rows)
+
+update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; -- ERROR
+ERROR: cannot route tuples into foreign table to be updated "remp"
drop table utrtest;
drop table loct;
-- Test copy tuple routing
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 25e219dd82..794363c184 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -185,6 +185,10 @@ typedef struct PgFdwModifyState
/* working memory context */
MemoryContext temp_cxt; /* context for per-tuple temporary data */
+
+ /* for update row movement if subplan result rel */
+ struct PgFdwModifyState *aux_fmstate; /* foreign-insert state, if
+ * created */
} PgFdwModifyState;
/*
@@ -1844,8 +1848,22 @@ postgresExecForeignInsert(EState *estate,
TupleTableSlot *slot,
TupleTableSlot *planSlot)
{
- return execute_foreign_modify(estate, resultRelInfo, CMD_INSERT,
+ PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
+ TupleTableSlot *rslot;
+
+ /*
+ * If the fmstate has aux_fmstate set, use the aux_fmstate (see
+ * postgresBeginForeignInsert())
+ */
+ if (fmstate->aux_fmstate)
+ resultRelInfo->ri_FdwState = fmstate->aux_fmstate;
+ rslot = execute_foreign_modify(estate, resultRelInfo, CMD_INSERT,
slot, planSlot);
+ /* Revert that change */
+ if (fmstate->aux_fmstate)
+ resultRelInfo->ri_FdwState = fmstate;
+
+ return rslot;
}
/*
@@ -1915,6 +1933,22 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
List *retrieved_attrs = NIL;
bool doNothing = false;
+ /*
+ * If the foreign table we are about to insert routed rows into is also
+ * an UPDATE subplan result rel that will be updated later, proceeding
+ * with the INSERT will result in the later UPDATE incorrectly modifying
+ * those routed rows, so prevent the INSERT --- it would be nice if we
+ * could handle this case; but for now, throw an error for safety.
+ */
+ if (plan && plan->operation == CMD_UPDATE &&
+ (resultRelInfo->ri_usesFdwDirectModify ||
+ resultRelInfo->ri_FdwState) &&
+ resultRelInfo > mtstate->resultRelInfo + mtstate->mt_whichplan)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot route tuples into foreign table to be updated \"%s\"",
+ RelationGetRelationName(rel))));
+
initStringInfo(&sql);
/* We transmit all columns that are defined in the foreign table. */
@@ -1983,7 +2017,19 @@ postgresBeginForeignInsert(ModifyTableState *mtstate,
retrieved_attrs != NIL,
retrieved_attrs);
- resultRelInfo->ri_FdwState = fmstate;
+ /*
+ * If the given resultRelInfo already has PgFdwModifyState set, it means
+ * the foreign table is an UPDATE subplan result rel; in which case, store
+ * the resulting state into the aux_fmstate of the PgFdwModifyState.
+ */
+ if (resultRelInfo->ri_FdwState)
+ {
+ Assert(plan && plan->operation == CMD_UPDATE);
+ Assert(resultRelInfo->ri_usesFdwDirectModify == false);
+ ((PgFdwModifyState *) resultRelInfo->ri_FdwState)->aux_fmstate = fmstate;
+ }
+ else
+ resultRelInfo->ri_FdwState = fmstate;
}
/*
@@ -1998,6 +2044,13 @@ postgresEndForeignInsert(EState *estate,
Assert(fmstate != NULL);
+ /*
+ * If the fmstate has aux_fmstate set, get the aux_fmstate (see
+ * postgresBeginForeignInsert())
+ */
+ if (fmstate->aux_fmstate)
+ fmstate = fmstate->aux_fmstate;
+
/* Destroy the execution state */
finish_foreign_modify(fmstate);
}
@@ -3482,6 +3535,9 @@ create_foreign_modify(EState *estate,
Assert(fmstate->p_nums <= n_params);
+ /* Initialize auxiliary state */
+ fmstate->aux_fmstate = NULL;
+
return fmstate;
}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 3bfcdabc78..127cd30a92 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2015,6 +2015,51 @@ update utrtest set a = 1 where a = 2 returning *;
drop trigger loct_br_insert_trigger on loct;
+-- We can move rows to a foreign partition that has been updated already,
+-- but can't move rows to a foreign partition that hasn't been updated yet
+
+delete from utrtest;
+insert into utrtest values (1, 'foo');
+insert into utrtest values (2, 'qux');
+
+-- Test the former case:
+-- with a direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 1 returning *;
+update utrtest set a = 1 returning *;
+
+delete from utrtest;
+insert into utrtest values (1, 'foo');
+insert into utrtest values (2, 'qux');
+
+-- with a non-direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
+update utrtest set a = 1 from (values (1), (2)) s(x) where a = s.x returning *;
+
+-- Change the definition of utrtest so that the foreign partition get updated
+-- after the local partition
+delete from utrtest;
+alter table utrtest detach partition remp;
+drop foreign table remp;
+alter table loct drop constraint loct_a_check;
+alter table loct add check (a in (3));
+create foreign table remp (a int check (a in (3)), b text) server loopback options (table_name 'loct');
+alter table utrtest attach partition remp for values in (3);
+insert into utrtest values (2, 'qux');
+insert into utrtest values (3, 'xyzzy');
+
+-- Test the latter case:
+-- with a direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 3 returning *;
+update utrtest set a = 3 returning *; -- ERROR
+
+-- with a non-direct modification plan
+explain (verbose, costs off)
+update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *;
+update utrtest set a = 3 from (values (2), (3)) s(x) where a = s.x returning *; -- ERROR
+
drop table utrtest;
drop table loct;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index a46fd750f6..e9ce39af64 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -74,6 +74,11 @@
UPDATE</literal> clause. However, the <literal>ON CONFLICT DO NOTHING</literal>
clause is supported, provided a unique index inference specification
is omitted.
+ Note also that <filename>postgres_fdw</filename> supports row movement
+ invoked by <command>UPDATE</command> statements executed on partitioned
+ tables, but it currently does not handle the case where a remote partition
+ chosen to insert a moved row into is also an <command>UPDATE</command>
+ target partition that will be updated later.
</para>
<para>
--
2.19.2
