From baf9b08711a8854ee48bbeef7e66c372aba61933 Mon Sep 17 00:00:00 2001 From: amitlan Date: Sat, 23 Jan 2021 16:35:21 +0900 Subject: [PATCH v2] 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 | 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 b4a04d2c14..666e597f54 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9251,5 +9251,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 8648be0b81..a9eeeb87f4 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 28b82f5f9d..8435817ea2 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2829,5 +2829,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 1746cb8793..882397bc30 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