From 46eefa1d915cb16abc48666e5098babc9cb94150 Mon Sep 17 00:00:00 2001 From: amitlan Date: Sat, 23 Jan 2021 16:35:21 +0900 Subject: [PATCH v3] Prevent FDW insert batching during cross-partition updates A cross-partition update of a partitioned table is internally implemented as delete+insert. As things stand now, even those inserts inadvertently end up using batching, because ExecInitRoutingInfo() that is in charge of initializing ri_BatchSize for partitions doesn't check the original operation, which would be CMD_UPDATE. That may pose a problem if the target of such an insert is a foreign partition, because its FDW may not be able to handle both the original update and the batched insert being performed at the same time. Prevent insert batching in such cases, for now. --- .../postgres_fdw/expected/postgres_fdw.out | 23 ++++++++++++++++++- contrib/postgres_fdw/postgres_fdw.c | 12 ++++++++-- contrib/postgres_fdw/sql/postgres_fdw.sql | 19 ++++++++++++++- src/backend/executor/execPartition.c | 3 ++- 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index b09dce63f5..f2540c614a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9396,5 +9396,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 2ce42ce3f1..993c6eb9d7 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -1934,17 +1934,25 @@ static int postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo) { int batch_size; + PgFdwModifyState *fmstate = + (PgFdwModifyState *) resultRelInfo->ri_FdwState; /* 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 319c15d635..6329c33946 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2897,5 +2897,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 746cd1e9d7..5bba6c42c1 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