On Tue, Feb 16, 2021 at 1:36 AM Tomas Vondra
<tomas.von...@enterprisedb.com> wrote:
> On 2/5/21 3:52 AM, Amit Langote wrote:
> > Tsunakwa-san,
> >
> > On Mon, Jan 25, 2021 at 1:21 PM tsunakawa.ta...@fujitsu.com
> > <tsunakawa.ta...@fujitsu.com> wrote:
> >> From: Amit Langote <amitlangot...@gmail.com>
> >>> Yes, it can be simplified by using a local join to prevent the update of
> >>> the foreign
> >>> partition from being pushed to the remote server, for which my example in
> >>> the
> >>> previous email used a local trigger. Note that the update of the foreign
> >>> partition to be done locally is a prerequisite for this bug to occur.
> >>
> >> Thank you, I was aware that UPDATE calls ExecInsert() but forgot about it
> >> partway. Good catch (and my bad miss.)
> >
> > It appears I had missed your reply, sorry.
> >
> >> + PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
> >> + (PgFdwModifyState
> >> *) resultRelInfo->ri_FdwState :
> >> + NULL;
> >>
> >> This can be written as:
> >>
> >> + PgFdwModifyState *fmstate = (PgFdwModifyState *)
> >> resultRelInfo->ri_FdwState;
> >
> > Facepalm, yes.
> >
> > Patch updated. Thanks for the review.
> >
>
> Thanks for the patch, it seems fine to me.
Thanks for checking.
> I wonder it the commit
> message needs some tweaks, though. At the moment it says:
>
> Prevent FDW insert batching during cross-partition updates
>
> but what the patch seems to be doing is simply initializing the info
> only for CMD_INSERT operations. Which does the trick, but it affects
> everything, i.e. all updates, no? Not just cross-partition updates.
You're right. Please check the message in the updated patch.
--
Amit Langote
EDB: http://www.enterprisedb.com
From b1d470fc764279ba12787271a04015a123d20b4f Mon Sep 17 00:00:00 2001
From: amitlan <amitlangote09@gmail.com>
Date: Sat, 23 Jan 2021 16:35:21 +0900
Subject: [PATCH v4] Fix tuple routing to initialize batching only for inserts
Currently, the insert component of a cross-partition update of a
partitioned table (internally implemented as a delete followed by an
insert) inadvertently ends up using batching for the insert. That may
pose a problem if the insert target is a foreign partition, because
the partition's FDW may not be able to handle both the original update
operation and the batched insert operation being performed at the same
time. So tighten up the check in ExecInitRoutingInfo() to initialize
batching only if the query's original operation is also INSERT.
---
.../postgres_fdw/expected/postgres_fdw.out | 23 ++++++++++++++++++-
contrib/postgres_fdw/postgres_fdw.c | 13 +++++++++--
contrib/postgres_fdw/sql/postgres_fdw.sql | 19 ++++++++++++++-
src/backend/executor/execPartition.c | 3 ++-
4 files changed, 53 insertions(+), 5 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 60c7e115d6..3326f1b542 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9414,5 +9414,26 @@ SELECT COUNT(*) FROM batch_table;
66
(1 row)
+-- Check that enabling batched inserts doesn't interfere with cross-partition
+-- updates
+CREATE TABLE batch_cp_upd_test (a int) PARTITION BY LIST (a);
+CREATE TABLE batch_cp_upd_test1 (LIKE batch_cp_upd_test);
+CREATE FOREIGN TABLE batch_cp_upd_test1_f
+ PARTITION OF batch_cp_upd_test
+ FOR VALUES IN (1)
+ SERVER loopback
+ OPTIONS (table_name 'batch_cp_upd_test1', batch_size '10');
+CREATE TABLE batch_cp_up_test1 PARTITION OF batch_cp_upd_test
+ FOR VALUES IN (2);
+INSERT INTO batch_cp_upd_test VALUES (1), (2);
+-- The following moves a row from the local partition to the foreign one
+UPDATE batch_cp_upd_test t SET a = 1 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a;
+SELECT tableoid::regclass, * FROM batch_cp_upd_test;
+ tableoid | a
+----------------------+---
+ batch_cp_upd_test1_f | 1
+ batch_cp_upd_test1_f | 1
+(2 rows)
+
-- Clean up
-DROP TABLE batch_table CASCADE;
+DROP TABLE batch_table, batch_cp_upd_test CASCADE;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 368997d9d1..35b48575c5 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1934,17 +1934,26 @@ static int
postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo)
{
int batch_size;
+ PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ?
+ (PgFdwModifyState *) resultRelInfo->ri_FdwState :
+ NULL;
/* should be called only once */
Assert(resultRelInfo->ri_BatchSize == 0);
+ /*
+ * Should never get called when the insert is being performed as part of
+ * a row movement operation.
+ */
+ Assert(fmstate == NULL || fmstate->aux_fmstate == NULL);
+
/*
* In EXPLAIN without ANALYZE, ri_fdwstate is NULL, so we have to lookup
* the option directly in server/table options. Otherwise just use the
* value we determined earlier.
*/
- if (resultRelInfo->ri_FdwState)
- batch_size = ((PgFdwModifyState *) resultRelInfo->ri_FdwState)->batch_size;
+ if (fmstate)
+ batch_size = fmstate->batch_size;
else
batch_size = get_batch_size_option(resultRelInfo->ri_RelationDesc);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 151f4f1834..2b525ea44a 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2909,5 +2909,22 @@ CREATE TABLE batch_table_p2
INSERT INTO batch_table SELECT * FROM generate_series(1, 66) i;
SELECT COUNT(*) FROM batch_table;
+-- Check that enabling batched inserts doesn't interfere with cross-partition
+-- updates
+CREATE TABLE batch_cp_upd_test (a int) PARTITION BY LIST (a);
+CREATE TABLE batch_cp_upd_test1 (LIKE batch_cp_upd_test);
+CREATE FOREIGN TABLE batch_cp_upd_test1_f
+ PARTITION OF batch_cp_upd_test
+ FOR VALUES IN (1)
+ SERVER loopback
+ OPTIONS (table_name 'batch_cp_upd_test1', batch_size '10');
+CREATE TABLE batch_cp_up_test1 PARTITION OF batch_cp_upd_test
+ FOR VALUES IN (2);
+INSERT INTO batch_cp_upd_test VALUES (1), (2);
+
+-- The following moves a row from the local partition to the foreign one
+UPDATE batch_cp_upd_test t SET a = 1 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a;
+SELECT tableoid::regclass, * FROM batch_cp_upd_test;
+
-- Clean up
-DROP TABLE batch_table CASCADE;
+DROP TABLE batch_table, batch_cp_upd_test CASCADE;
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index b9e4f2d80b..b8da4c5967 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1000,7 +1000,8 @@ ExecInitRoutingInfo(ModifyTableState *mtstate,
*
* If the FDW does not support batching, we set the batch size to 1.
*/
- if (partRelInfo->ri_FdwRoutine != NULL &&
+ if (mtstate->operation == CMD_INSERT &&
+ partRelInfo->ri_FdwRoutine != NULL &&
partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize &&
partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert)
partRelInfo->ri_BatchSize =
--
2.24.1
