From 21480f1c6b858877e45919d551dc47a85bb2a89f Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Thu, 12 Nov 2026 19:26:11 +0100 Subject: [PATCH v1 1/4] Rework batching decision for foreign tables Previously, it was up to the FDW to decide whether to support batching, and choose the batch size accordingly. In retrospect, that's not quite right - it really should be the responsibility of nodeModifyTable. This commit moves the decision to the core code, disabling batching depending on the features of the target table. It's also tightening the conditions a little bit, by disabling batching in additional cases. We can't allow batching in cases that require slots with the remote data, which we may not have: - AFTER ROW triggers - AFTER STATEMENT triggers (with a transition table) - WITH CHECK OPTION - RETURNING The existing code failed to consider transition tables on the parent table when routing tuples to a foreign partition, so fix that. We allow plain AFTER STATEMENT triggers, which happen after flushing all pending inserts. Furthermore, we can't allow BEFORE ROW triggers. The triggers might query the target table, which would be a problem for COPY (as the trigger would not see the buffered tuples). The INSERT case addresses that by flushing tuples before each BEFORE ROW triggers, but that effectively disables batching (flushing tuples one by one). With the above restrictions we do not need to access the slots (returned by ExecForeignBatchInsert) at all. The number of slots returned by the callback is used only to count processed slots (es_processed). We expect the value to be the number of processed input slots, which is what the non-batching code counts too. A server may still return a bogus value, but we at least clamp the returned value. There's not much we can do. Disabling batching for transition tables is a user-visible behavior change. A cross-partition UPDATE moving rows to a foreign partition with batching enabled, on a partitioned table with an AFTER UPDATE trigger with a NEW TABLE, used to succeed, but the moved rows were silently missing from the NEW TABLE. Now the UPDATE fails with "cannot collect transition tuples from child foreign tables", which is what happened without batching (batch_size=1). An error is better than an incomplete transition table. With only an OLD TABLE, the UPDATE still succeeds, but the moved rows are no longer batched. INSERT and COPY routing tuples to a foreign partition with an INSERT NEW TABLE already failed this way, even with batching enabled, so those are not affected. MERGE prepares transition capture for all kinds of actions, so with any transition table, batching is now disabled even for tuples routed to a foreign partition by an INSERT action. That does not fail, but may be slower. COPY FROM now clamps the batch size returned by the FDW to at least 1. Values below 1 were caught only by an assert, and with a bogus value (e.g. 0), CopyMultiInsertBufferFlush() would loop forever. XXX Maybe it should simply use an assert, like the other callers? Updates the SGML documentation for ExecForeignBatchInsert, to clarify in which cases is the batching disabled (not by the FDW). Add regression tests verifying that batching actually gets disabled in the cases listed above. The tests also show the behavior change for UPDATE and MERGE with transition tables. --- .../postgres_fdw/expected/postgres_fdw.out | 201 ++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.c | 20 +- contrib/postgres_fdw/sql/postgres_fdw.sql | 115 ++++++++++ doc/src/sgml/fdwhandler.sgml | 37 ++-- src/backend/commands/copyfrom.c | 77 ++++--- src/backend/executor/execPartition.c | 33 ++- src/backend/executor/nodeModifyTable.c | 87 ++++---- 7 files changed, 459 insertions(+), 111 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index b3c73b83462..35def829d31 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12117,6 +12117,207 @@ SELECT * FROM cmdlog ORDER BY 1; INSERT on batch_cp_upd_test3 (3 rows) +-- AFTER STATEMENT triggers without transition tables must not disable +-- batching, for either direct inserts or tuple routing. With batching +-- active we expect only one entry for batch_cp_upd_test1, as both +-- rows get inserted in a single statement. +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test1_f + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + cmd +-------------------------------- + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1_f +(2 rows) + +TRUNCATE cmdlog; +COPY batch_cp_upd_test1_f FROM STDIN; +SELECT * FROM cmdlog ORDER BY 1; + cmd +-------------------------------- + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1_f +(2 rows) + +-- A statement targeting the parent fires its trigger, not the leaf's. +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test + INSERT on batch_cp_upd_test1 +(2 rows) + +TRUNCATE cmdlog; +COPY batch_cp_upd_test FROM STDIN; +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test + INSERT on batch_cp_upd_test1 +(2 rows) + +DROP TRIGGER stmt_trig ON batch_cp_upd_test1_f; +DROP TRIGGER stmt_trig ON batch_cp_upd_test; +-- AFTER ROW triggers must disable batching and fire for each inserted row. +-- Each two-row command should log two remote INSERTs. +CREATE TRIGGER row_trig AFTER INSERT ON batch_cp_upd_test1_f + FOR EACH ROW EXECUTE FUNCTION trigger_data(23,'skidoo'); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1); +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +TRUNCATE cmdlog; +COPY batch_cp_upd_test1_f FROM STDIN; +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1); +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +TRUNCATE cmdlog; +COPY batch_cp_upd_test FROM STDIN; +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +NOTICE: row_trig(23, skidoo) AFTER ROW INSERT ON batch_cp_upd_test1_f +NOTICE: NEW: (1) +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +DROP TRIGGER row_trig ON batch_cp_upd_test1_f; +-- RETURNING must disable batching even without local triggers. +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1) RETURNING a; + a +--- + 1 + 1 +(2 rows) + +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1) RETURNING a; + a +--- + 1 + 1 +(2 rows) + +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +-- WITH CHECK OPTION must disable batching independently of RETURNING. +CREATE VIEW batch_cp_upd_view AS + SELECT * FROM batch_cp_upd_test1_f WHERE a = 1 WITH CHECK OPTION; +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_view VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +CREATE OR REPLACE VIEW batch_cp_upd_view AS + SELECT * FROM batch_cp_upd_test WHERE a = 1 WITH CHECK OPTION; +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_view VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +DROP VIEW batch_cp_upd_view; +-- Transition tables on the parent disable batching for the rows routed to the +-- foreign partition, even if there's only an OLD TABLE: expect an INSERT on +-- batch_cp_upd_test1 for each moved row. +CREATE TRIGGER oldtab_trig AFTER UPDATE ON batch_cp_upd_test + REFERENCING OLD TABLE AS oldtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +INSERT INTO batch_cp_upd_test VALUES (2), (2); +TRUNCATE cmdlog; +UPDATE batch_cp_upd_test SET a = 1 WHERE a = 2; +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 + UPDATE on batch_cp_upd_test +(3 rows) + +DROP TRIGGER oldtab_trig ON batch_cp_upd_test; +-- With a NEW TABLE, batching is disabled, and the per-row code refuses to +-- collect the rows moved to the foreign partition. +CREATE TRIGGER newtab_trig AFTER UPDATE ON batch_cp_upd_test + REFERENCING NEW TABLE AS newtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +INSERT INTO batch_cp_upd_test VALUES (2), (2); +UPDATE batch_cp_upd_test SET a = 1 WHERE a = 2; +ERROR: cannot collect transition tuples from child foreign tables +-- MERGE prepares transition capture for all kinds of actions, so the UPDATE +-- NEW TABLE disables batching even for rows routed by an INSERT action: +-- expect two INSERTs on batch_cp_upd_test1. (The condition on t.a prunes the +-- foreign partitions, which MERGE does not support as targets.) +TRUNCATE cmdlog; +MERGE INTO batch_cp_upd_test t USING (VALUES (1), (1)) s(a) + ON t.a = 2 AND t.a = s.a + WHEN NOT MATCHED THEN INSERT VALUES (s.a); +SELECT * FROM cmdlog ORDER BY 1; + cmd +------------------------------ + INSERT on batch_cp_upd_test1 + INSERT on batch_cp_upd_test1 +(2 rows) + +DROP TRIGGER newtab_trig ON batch_cp_upd_test; -- Clean up DROP TABLE batch_cp_upd_test; DROP TABLE batch_cp_upd_test1; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 7621f679a99..c01b517cd27 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -2251,8 +2251,7 @@ postgresExecForeignBatchInsert(EState *estate, * Determine the maximum number of tuples that can be inserted in bulk * * Returns the batch size specified for server or table. When batching is not - * allowed (e.g. for tables with BEFORE/AFTER ROW triggers or with RETURNING - * clause), returns 1. + * allowed (e.g. for tables with no columns), returns 1. */ static int postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo) @@ -2280,23 +2279,6 @@ postgresGetForeignModifyBatchSize(ResultRelInfo *resultRelInfo) else batch_size = get_batch_size_option(resultRelInfo->ri_RelationDesc); - /* - * Disable batching when we have to use RETURNING, there are any - * BEFORE/AFTER ROW INSERT triggers on the foreign table, or there are any - * WITH CHECK OPTION constraints from parent views. - * - * When there are any BEFORE ROW INSERT triggers on the table, we can't - * support it, because such triggers might query the table we're inserting - * into and act differently if the tuples that have already been processed - * and prepared for insertion are not there. - */ - if (resultRelInfo->ri_projectReturning != NULL || - resultRelInfo->ri_WithCheckOptions != NIL || - (resultRelInfo->ri_TrigDesc && - (resultRelInfo->ri_TrigDesc->trig_insert_before_row || - resultRelInfo->ri_TrigDesc->trig_insert_after_row))) - return 1; - /* * If the foreign table has no columns, disable batching as the INSERT * syntax doesn't allow batching multiple empty rows into a zero-column diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 8fc564ac37d..3ee739d40ef 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4174,6 +4174,121 @@ SELECT tableoid::regclass, * FROM batch_cp_upd_test ORDER BY 1; -- described above. SELECT * FROM cmdlog ORDER BY 1; +-- AFTER STATEMENT triggers without transition tables must not disable +-- batching, for either direct inserts or tuple routing. With batching +-- active we expect only one entry for batch_cp_upd_test1, as both +-- rows get inserted in a single statement. +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test1_f + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +COPY batch_cp_upd_test1_f FROM STDIN; +1 +1 +\. +SELECT * FROM cmdlog ORDER BY 1; + +-- A statement targeting the parent fires its trigger, not the leaf's. +CREATE TRIGGER stmt_trig AFTER INSERT ON batch_cp_upd_test + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +COPY batch_cp_upd_test FROM STDIN; +1 +1 +\. +SELECT * FROM cmdlog ORDER BY 1; + +DROP TRIGGER stmt_trig ON batch_cp_upd_test1_f; +DROP TRIGGER stmt_trig ON batch_cp_upd_test; + +-- AFTER ROW triggers must disable batching and fire for each inserted row. +-- Each two-row command should log two remote INSERTs. +CREATE TRIGGER row_trig AFTER INSERT ON batch_cp_upd_test1_f + FOR EACH ROW EXECUTE FUNCTION trigger_data(23,'skidoo'); +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +COPY batch_cp_upd_test1_f FROM STDIN; +1 +1 +\. +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +COPY batch_cp_upd_test FROM STDIN; +1 +1 +\. +SELECT * FROM cmdlog ORDER BY 1; +DROP TRIGGER row_trig ON batch_cp_upd_test1_f; + +-- RETURNING must disable batching even without local triggers. +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test1_f VALUES (1), (1) RETURNING a; +SELECT * FROM cmdlog ORDER BY 1; + +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_test VALUES (1), (1) RETURNING a; +SELECT * FROM cmdlog ORDER BY 1; + +-- WITH CHECK OPTION must disable batching independently of RETURNING. +CREATE VIEW batch_cp_upd_view AS + SELECT * FROM batch_cp_upd_test1_f WHERE a = 1 WITH CHECK OPTION; +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_view VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; + +CREATE OR REPLACE VIEW batch_cp_upd_view AS + SELECT * FROM batch_cp_upd_test WHERE a = 1 WITH CHECK OPTION; +TRUNCATE cmdlog; +INSERT INTO batch_cp_upd_view VALUES (1), (1); +SELECT * FROM cmdlog ORDER BY 1; +DROP VIEW batch_cp_upd_view; + +-- Transition tables on the parent disable batching for the rows routed to the +-- foreign partition, even if there's only an OLD TABLE: expect an INSERT on +-- batch_cp_upd_test1 for each moved row. +CREATE TRIGGER oldtab_trig AFTER UPDATE ON batch_cp_upd_test + REFERENCING OLD TABLE AS oldtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +INSERT INTO batch_cp_upd_test VALUES (2), (2); +TRUNCATE cmdlog; +UPDATE batch_cp_upd_test SET a = 1 WHERE a = 2; +SELECT * FROM cmdlog ORDER BY 1; +DROP TRIGGER oldtab_trig ON batch_cp_upd_test; + +-- With a NEW TABLE, batching is disabled, and the per-row code refuses to +-- collect the rows moved to the foreign partition. +CREATE TRIGGER newtab_trig AFTER UPDATE ON batch_cp_upd_test + REFERENCING NEW TABLE AS newtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +INSERT INTO batch_cp_upd_test VALUES (2), (2); +UPDATE batch_cp_upd_test SET a = 1 WHERE a = 2; + +-- MERGE prepares transition capture for all kinds of actions, so the UPDATE +-- NEW TABLE disables batching even for rows routed by an INSERT action: +-- expect two INSERTs on batch_cp_upd_test1. (The condition on t.a prunes the +-- foreign partitions, which MERGE does not support as targets.) +TRUNCATE cmdlog; +MERGE INTO batch_cp_upd_test t USING (VALUES (1), (1)) s(a) + ON t.a = 2 AND t.a = s.a + WHEN NOT MATCHED THEN INSERT VALUES (s.a); +SELECT * FROM cmdlog ORDER BY 1; +DROP TRIGGER newtab_trig ON batch_cp_upd_test; + -- Clean up DROP TABLE batch_cp_upd_test; DROP TABLE batch_cp_upd_test1; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 502441fefcb..ea288520dfc 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -661,22 +661,24 @@ ExecForeignBatchInsert(EState *estate, - The return value is an array of slots containing the data that was - actually inserted (this might differ from the data supplied, for - example as a result of trigger actions.) - The passed-in slots can be re-used for this purpose. - The number of successfully inserted tuples is returned in - *numSlots. + The function must set *numSlots to the number of + successfully inserted tuples. This can be fewer than the number of + tuples passed in, or even zero, for example when + ON CONFLICT DO NOTHING skips conflicting tuples. + The executor ignores the returned slot array, so the function can return + NULL. It need not populate slots with the data that + was actually inserted. - The data in the returned slot is used only if the INSERT - statement involves a view - WITH CHECK OPTION; or if the foreign table has - an AFTER ROW trigger. Triggers require all columns, - but the FDW could choose to optimize away returning some or all columns - depending on the contents of the - WITH CHECK OPTION constraints. + The executor uses ExecForeignInsert instead when + the statement has a RETURNING clause, there are + WITH CHECK OPTION constraints to check, or the foreign + table has BEFORE ROW or AFTER ROW + INSERT triggers. Batching is also disabled when + transition-tuple capture is active for a parent table during tuple + routing. Statement-level triggers without transition tables do not + prevent batching. @@ -684,8 +686,6 @@ ExecForeignBatchInsert(EState *estate, GetForeignModifyBatchSize pointer is set to NULL, attempts to insert into the foreign table will use ExecForeignInsert. - This function is not used if the INSERT has the - RETURNING clause. @@ -710,6 +710,13 @@ GetForeignModifyBatchSize(ResultRelInfo *rinfo); the target foreign table. The FDW is expected to provide a foreign server and/or foreign table option for the user to set this value, or some hard-coded value. + The return value must be at least one; returning one disables batching. + + + + This function is not called when the executor has disabled batching for + one of the reasons described for + ExecForeignBatchInsert. diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index 89f48c8951b..b75625e65ff 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -467,6 +467,16 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo, Assert(resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert); Assert(batch_size > 1); + /* Triggers on foreign tables should not have transition tables */ + Assert(resultRelInfo->ri_TrigDesc == NULL || + resultRelInfo->ri_TrigDesc->trig_insert_new_table == false); + + /* Batching does not allow BEFORE ROW or AFTER ROW INSERT triggers */ + Assert(resultRelInfo->ri_TrigDesc == NULL || + resultRelInfo->ri_TrigDesc->trig_insert_before_row == false); + Assert(resultRelInfo->ri_TrigDesc == NULL || + resultRelInfo->ri_TrigDesc->trig_insert_after_row == false); + /* * We suppress error context information other than the relation name, * if one of the operations below fails. @@ -478,15 +488,13 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo, { int size = (batch_size < nused - sent) ? batch_size : (nused - sent); int inserted = size; - TupleTableSlot **rslots; /* insert into foreign table: let the FDW do it */ - rslots = - resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate, - resultRelInfo, - &slots[sent], - NULL, - &inserted); + resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate, + resultRelInfo, + &slots[sent], + NULL, + &inserted); /* XXX See the comment in ExecBatchInsert() */ inserted = Min(inserted, size); @@ -497,32 +505,6 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo, if (inserted <= 0) continue; - /* Triggers on foreign tables should not have transition tables */ - Assert(resultRelInfo->ri_TrigDesc == NULL || - resultRelInfo->ri_TrigDesc->trig_insert_new_table == false); - - /* Run AFTER ROW INSERT triggers */ - if (resultRelInfo->ri_TrigDesc != NULL && - resultRelInfo->ri_TrigDesc->trig_insert_after_row) - { - Oid relid = RelationGetRelid(resultRelInfo->ri_RelationDesc); - - for (i = 0; i < inserted; i++) - { - TupleTableSlot *slot = rslots[i]; - - /* - * AFTER ROW Triggers might reference the tableoid column, - * so (re-)initialize tts_tableOid before evaluating them. - */ - slot->tts_tableOid = relid; - - ExecARInsertTriggers(estate, resultRelInfo, - slot, NULL, NIL, - cstate->transition_capture); - } - } - /* Update the row counter and progress of the COPY command */ *processed += inserted; pgstat_progress_update_param(PROGRESS_COPY_TUPLES_PROCESSED, @@ -804,6 +786,7 @@ CopyFrom(CopyFromState cstate) bool has_before_insert_row_trig; bool has_instead_insert_row_trig; bool leafpart_use_multi_insert = false; + bool allow_batching = true; Assert(cstate->rel); Assert(list_length(cstate->range_table) == 1); @@ -945,6 +928,25 @@ CopyFrom(CopyFromState cstate) resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, resultRelInfo); + /* + * Disable batching when we have to use RETURNING, there are any + * BEFORE/AFTER ROW INSERT triggers on the foreign table, or there are any + * WITH CHECK OPTION constraints from parent views. + * + * It might seem we can allow BEFORE ROW INSERT triggers on the table, but + * such triggers might query the table we're inserting into and misbehave + * due to not finding processed but not-yet-flushed tuples. + * + * FIXME We have this check in multiple places, we should replace it with + * some helper. + */ + if (resultRelInfo->ri_projectReturning != NULL || + resultRelInfo->ri_WithCheckOptions != NIL || + (resultRelInfo->ri_TrigDesc && + (resultRelInfo->ri_TrigDesc->trig_insert_before_row || + resultRelInfo->ri_TrigDesc->trig_insert_after_row))) + allow_batching = false; + /* * Also, if the named relation is a foreign table, determine if the FDW * supports batch insert and determine the batch size (a FDW may support @@ -954,13 +956,18 @@ CopyFrom(CopyFromState cstate) */ if (resultRelInfo->ri_FdwRoutine != NULL && resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && - resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) + resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert && + allow_batching) resultRelInfo->ri_BatchSize = resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo); else resultRelInfo->ri_BatchSize = 1; - Assert(resultRelInfo->ri_BatchSize >= 1); + /* + * Don't rely on the FDW to return a valid batch size. With a batch size + * below 1, CopyMultiInsertBufferFlush() would loop forever. + */ + resultRelInfo->ri_BatchSize = Max(resultRelInfo->ri_BatchSize, 1); /* Prepare to catch AFTER triggers. */ AfterTriggerBeginQuery(); diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index 4ec69f2bf99..cd0d20aa3c1 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -1113,6 +1113,7 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, { MemoryContext oldcxt; int rri_index; + bool allow_batching = true; oldcxt = MemoryContextSwitchTo(proute->memcxt); @@ -1145,6 +1146,35 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL) partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo); + /* + * Disable batching when we have to use RETURNING, there are any + * BEFORE/AFTER ROW INSERT triggers on the foreign table, or there are any + * WITH CHECK OPTION constraints from parent views. + * + * The parent may have triggers with transition tables even if the + * partition has no triggers. Disable batching in that case, too, so the + * per-row trigger code can reject collecting transition tuples from + * foreign partitions. + * + * FIXME Another copy of this check, we should replace it with a helper + * (with an additional condition on the parent transition capture). + * + * FIXME I'm not 100% sure about the combination of partRelInfo and + * mtstate in this check. Are there other combinations we should check? + * + * FIXME The mtstate->mt_transition_capture also disables batching for OLD + * TABLE-only UPDATE triggers. This disables batching anytime the + * transition capture is active - a performance regression. Maybe that's + * too strict and we should check if tcs_update_new_table is set? + */ + if (partRelInfo->ri_projectReturning != NULL || + partRelInfo->ri_WithCheckOptions != NIL || + mtstate->mt_transition_capture || + (partRelInfo->ri_TrigDesc && + (partRelInfo->ri_TrigDesc->trig_insert_before_row || + partRelInfo->ri_TrigDesc->trig_insert_after_row))) + allow_batching = false; + /* * Determine if the FDW supports batch insert and determine the batch size * (a FDW may support batching, but it may be disabled for the @@ -1154,7 +1184,8 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, */ if (partRelInfo->ri_FdwRoutine != NULL && partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && - partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) + partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert && + allow_batching) partRelInfo->ri_BatchSize = partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(partRelInfo); else diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index cb73cc61821..a78aaaea6dd 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -137,8 +137,7 @@ typedef struct UpdateContext } UpdateContext; -static void ExecBatchInsert(ModifyTableState *mtstate, - ResultRelInfo *resultRelInfo, +static void ExecBatchInsert(ResultRelInfo *resultRelInfo, TupleTableSlot **slots, TupleTableSlot **planSlots, int numSlots, @@ -962,7 +961,7 @@ ExecInsert(ModifyTableContext *context, */ if (resultRelInfo->ri_NumSlots == resultRelInfo->ri_BatchSize) { - ExecBatchInsert(mtstate, resultRelInfo, + ExecBatchInsert(resultRelInfo, resultRelInfo->ri_Slots, resultRelInfo->ri_PlanSlots, resultRelInfo->ri_NumSlots, @@ -1394,8 +1393,7 @@ ExecInsert(ModifyTableContext *context, * ---------------------------------------------------------------- */ static void -ExecBatchInsert(ModifyTableState *mtstate, - ResultRelInfo *resultRelInfo, +ExecBatchInsert(ResultRelInfo *resultRelInfo, TupleTableSlot **slots, TupleTableSlot **planSlots, int numSlots, @@ -1404,49 +1402,34 @@ ExecBatchInsert(ModifyTableState *mtstate, { int i; int numInserted = numSlots; - TupleTableSlot *slot = NULL; - TupleTableSlot **rslots; + + /* + * Batching does not allow RETURNING, WITH CHECK OPTION constraints, or + * BEFORE ROW and AFTER ROW INSERT triggers. + */ + Assert(resultRelInfo->ri_projectReturning == NULL); + Assert(resultRelInfo->ri_WithCheckOptions == NIL); + Assert(resultRelInfo->ri_TrigDesc == NULL || + !resultRelInfo->ri_TrigDesc->trig_insert_before_row); + Assert(resultRelInfo->ri_TrigDesc == NULL || + !resultRelInfo->ri_TrigDesc->trig_insert_after_row); /* * insert into foreign table: let the FDW do it */ - rslots = resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate, - resultRelInfo, - slots, - planSlots, - &numInserted); + resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert(estate, + resultRelInfo, + slots, + planSlots, + &numInserted); /* - * XXX An FDW may report more rows than we gave it, which would lead to - * buffer overruns in the loop below. We clamp the count so that - * statements that happen to work today keep working, at the cost of - * misreporting the number of rows. This is a stopgap; a proper fix means - * reworking the batch insert API. + * XXX An FDW may report more rows than we gave it. We only use the value + * to increment the number of processed tuples below, so we simply clamp + * it by the number of input slots. */ numInserted = Min(numInserted, numSlots); - for (i = 0; i < numInserted; i++) - { - slot = rslots[i]; - - /* - * AFTER ROW Triggers might reference the tableoid column, so - * (re-)initialize tts_tableOid before evaluating them. - */ - slot->tts_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc); - - /* AFTER ROW INSERT Triggers */ - ExecARInsertTriggers(estate, resultRelInfo, slot, NULL, NIL, - mtstate->mt_transition_capture); - - /* - * Check any WITH CHECK OPTION constraints from parent views. See the - * comment in ExecInsert. - */ - if (resultRelInfo->ri_WithCheckOptions != NIL) - ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate); - } - if (canSetTag && numInserted > 0) estate->es_processed += numInserted; @@ -1475,7 +1458,7 @@ ExecPendingInserts(EState *estate) ModifyTableState *mtstate = (ModifyTableState *) lfirst(l2); Assert(mtstate); - ExecBatchInsert(mtstate, resultRelInfo, + ExecBatchInsert(resultRelInfo, resultRelInfo->ri_Slots, resultRelInfo->ri_PlanSlots, resultRelInfo->ri_NumSlots, @@ -5474,13 +5457,35 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) */ if (operation == CMD_INSERT) { + bool allow_batching = true; + /* insert may only have one relation, inheritance is not expanded */ Assert(total_nrels == 1); resultRelInfo = mtstate->resultRelInfo; + + /* + * Disable batching when we have to use RETURNING, there are any + * BEFORE/AFTER ROW INSERT triggers on the foreign table, or there are + * any WITH CHECK OPTION constraints from parent views. + * + * We could allow batching for BEFORE ROW triggers, but it would be + * mostly useless. We would end up flushing the inserts before every + * row, which effectively disables batching. + * + * FIXME another copy of this check, replace with a helper somewhere + */ + if (resultRelInfo->ri_projectReturning != NULL || + resultRelInfo->ri_WithCheckOptions != NIL || + (resultRelInfo->ri_TrigDesc && + (resultRelInfo->ri_TrigDesc->trig_insert_before_row || + resultRelInfo->ri_TrigDesc->trig_insert_after_row))) + allow_batching = false; + if (!resultRelInfo->ri_usesFdwDirectModify && resultRelInfo->ri_FdwRoutine != NULL && resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && - resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert) + resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert && + allow_batching) { resultRelInfo->ri_BatchSize = resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo); -- 2.56.0