From f7eb527499686a2b49e470cfc1fe9b2a2dcac9df Mon Sep 17 00:00:00 2001 From: amitlan Date: Tue, 16 Mar 2021 14:03:05 +0900 Subject: [PATCH v11] Allow batching of inserts during cross-partition updates Currently, the insert that runs internally as part of cross-partition update of partitioned tables can't use batching because ExecInitRoutingInfo(), which initializes the insert target partition, allows batching only if the query's original operation is CMD_INSERT. This commit loosens that restriction to allow the internal inserts of cross-partition updates use batching too, essentially reverting a check that 927f453a94106 put in place. While at it, also update the comment in postgresGetForeignModifyBatchSize() a bit to clarify which inserts are disallowed during cross-partition updates. This also adjusts the test case added by 927f453a94106 to confirm that the insert performed when moving the rows into the foreign partition indeed uses batched mode. --- .../postgres_fdw/expected/postgres_fdw.out | 56 +++++++++++++++---- contrib/postgres_fdw/postgres_fdw.c | 10 ++-- contrib/postgres_fdw/sql/postgres_fdw.sql | 46 ++++++++++++--- src/backend/executor/execPartition.c | 3 +- 4 files changed, 89 insertions(+), 26 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2ab3f1efaa..e75d4f629a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -10345,8 +10345,8 @@ SELECT COUNT(*) FROM batch_table; 66 (1 row) --- Check that enabling batched inserts doesn't interfere with cross-partition --- updates +-- Check that batched mode also works for some inserts made during +-- 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 @@ -10354,21 +10354,55 @@ CREATE FOREIGN TABLE batch_cp_upd_test1_f 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 +CREATE TABLE batch_cp_upd_test2 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; -ERROR: cannot route tuples into foreign table to be updated "batch_cp_upd_test1_f" -SELECT tableoid::regclass, * FROM batch_cp_upd_test; +CREATE TABLE batch_cp_upd_test3 (LIKE batch_cp_upd_test); +CREATE FOREIGN TABLE batch_cp_upd_test3_f + PARTITION OF batch_cp_upd_test + FOR VALUES IN (3) + SERVER loopback + OPTIONS (table_name 'batch_cp_upd_test3', batch_size '1'); +INSERT INTO batch_cp_upd_test VALUES (2), (2); +-- Create statement triggers on remote tables that "log" any INSERTs +-- performed on them. +CREATE TABLE cmdlog (cmd text); +CREATE FUNCTION log_stmt () RETURNS TRIGGER LANGUAGE plpgsql AS $$ + BEGIN INSERT INTO public.cmdlog VALUES (TG_OP || ' on ' || TG_RELNAME); RETURN NULL; END; +$$; +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test1 + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test3 + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +-- This update moves rows from the local partition 'batch_cp_upd_test2' to the +-- foreign partition 'batch_cp_upd_test1', one that has insert batching enabled, +-- so a single INSERTs for both rows. +UPDATE batch_cp_upd_test t SET a = 1 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a AND s.a = 2; +-- This one moves rows from the local partition 'batch_cp_upd_test2' to the +-- foreign partition 'batch_cp_upd_test2', one that has insert batching +-- disabled, so separate INSERTs for the two rows. +INSERT INTO batch_cp_upd_test VALUES (2), (2); +UPDATE batch_cp_upd_test t SET a = 3 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a AND s.a = 2; +SELECT tableoid::regclass, * FROM batch_cp_upd_test ORDER BY 1; tableoid | a ----------------------+--- batch_cp_upd_test1_f | 1 - batch_cp_up_test1 | 2 -(2 rows) + batch_cp_upd_test1_f | 1 + batch_cp_upd_test3_f | 3 + batch_cp_upd_test3_f | 3 +(4 rows) + +-- Should see 1 INSERT on batch_cp_upd_test1 and 2 on batch_cp_upd_test3 as +-- described above. +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test3 + INSERT on batch_cp_upd_test3 +(3 rows) -- Clean up -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, batch_table_p1 CASCADE; +DROP TABLE batch_table, batch_table_p0, batch_table_p1, batch_cp_upd_test, cmdlog CASCADE; -- Use partitioning ALTER SERVER loopback OPTIONS (ADD batch_size '10'); CREATE TABLE batch_table ( x int, field1 text, field2 text) PARTITION BY HASH (x); diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 1ceac2e0cf..1f6d236426 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -2017,16 +2017,16 @@ static int postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo) { int batch_size; - PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState ? - (PgFdwModifyState *) resultRelInfo->ri_FdwState : - NULL; + 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. + * Should never get called when the insert is being performed on a table + * that is also among the target relations of an UPDATE operation, + * because postgresBeginForeignInsert() currently rejects such insert + * attempts. */ Assert(fmstate == NULL || fmstate->aux_fmstate == NULL); diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 51560429e0..3495666d07 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3327,8 +3327,8 @@ 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 +-- Check that batched mode also works for some inserts made during +-- 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 @@ -3336,16 +3336,46 @@ CREATE FOREIGN TABLE batch_cp_upd_test1_f 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 +CREATE TABLE batch_cp_upd_test2 PARTITION OF batch_cp_upd_test FOR VALUES IN (2); -INSERT INTO batch_cp_upd_test VALUES (1), (2); +CREATE TABLE batch_cp_upd_test3 (LIKE batch_cp_upd_test); +CREATE FOREIGN TABLE batch_cp_upd_test3_f + PARTITION OF batch_cp_upd_test + FOR VALUES IN (3) + SERVER loopback + OPTIONS (table_name 'batch_cp_upd_test3', batch_size '1'); +INSERT INTO batch_cp_upd_test VALUES (2), (2); + +-- Create statement triggers on remote tables that "log" any INSERTs +-- performed on them. +CREATE TABLE cmdlog (cmd text); +CREATE FUNCTION log_stmt () RETURNS TRIGGER LANGUAGE plpgsql AS $$ + BEGIN INSERT INTO public.cmdlog VALUES (TG_OP || ' on ' || TG_RELNAME); RETURN NULL; END; +$$; +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test1 + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test3 + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); + +-- This update moves rows from the local partition 'batch_cp_upd_test2' to the +-- foreign partition 'batch_cp_upd_test1', one that has insert batching enabled, +-- so a single INSERTs for both rows. +UPDATE batch_cp_upd_test t SET a = 1 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a AND s.a = 2; + +-- This one moves rows from the local partition 'batch_cp_upd_test2' to the +-- foreign partition 'batch_cp_upd_test2', one that has insert batching +-- disabled, so separate INSERTs for the two rows. +INSERT INTO batch_cp_upd_test VALUES (2), (2); +UPDATE batch_cp_upd_test t SET a = 3 FROM (VALUES (1), (2)) s(a) WHERE t.a = s.a AND s.a = 2; + +SELECT tableoid::regclass, * FROM batch_cp_upd_test ORDER BY 1; --- 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; +-- Should see 1 INSERT on batch_cp_upd_test1 and 2 on batch_cp_upd_test3 as +-- described above. +SELECT * FROM cmdlog ORDER BY 1; -- Clean up -DROP TABLE batch_table, batch_cp_upd_test, batch_table_p0, batch_table_p1 CASCADE; +DROP TABLE batch_table, batch_table_p0, batch_table_p1, batch_cp_upd_test, cmdlog CASCADE; -- Use partitioning ALTER SERVER loopback OPTIONS (ADD batch_size '10'); diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 88d0ea3adb..7a13c4dc9e 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -1018,8 +1018,7 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, * * If the FDW does not support batching, we set the batch size to 1. */ - if (mtstate->operation == CMD_INSERT && - partRelInfo->ri_FdwRoutine != NULL && + if (partRelInfo->ri_FdwRoutine != NULL && partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) partRelInfo->ri_BatchSize = -- 2.35.3