Hi, While working on one of the fixed for the new releases [1], it became clear the FDW insert batching (introduced in PG 14) has a couple weak points that we should address. The fix [1] is merely a stopgap, this patch series is an attempt to do that properly.
First, the API leaves the decision whether to enable batching completely up to the FDW. The FDW can enable batching even in cases when it's not really correct - for example with AFTER ROW triggers, when we need to know which individual rows were enabled (there are multiple such cases). In other cases (BEFORE ROW triggers) the FDW may enable batching, but the executor ends up flushing the buffer before each row, making the batching entirely useless. These are just examples, but I think it's very clear most of this decision should happen in core code. The FDW should have a say in what batch size it prefers, but it's up to the core to decide if batching can be enabled for the query. Then there is the question in which cases it's actually correct to enable insert batching. We can't really enable batching for cases that need to know the new tuples, like AFTER ROW triggers, or even AFTER STATEMENT triggers with transition tables. The old check disabled some of these, but not all of them. Before the fix this might cause crashes, now it means the query can silently drop the new tuples, run triggers on different tuples, etc. There's probably multiple different ways how it could go wrong. 0001 addresses both of these issues. It moves the batch size decision from postgres_fdw.c to core, and tightens the checks when to allow batching. It's not very pretty, because it copies the check into three different places, but 0002 addresses that. 0002 simply replaces the duplicated check with a simple helper, called from all those places. 0003 relaxes the conditions a little bit, by allowing batching in cases that need only OLD TABLE transition tables. Ultimately, all of these patches should be squashed into a single change. I kept them separate to make review easier. I wonder if maybe there are additional cases where we need to disable batching (or where we could enable it, similarly to 0003). It's quite possible I failed to consider something. I also am not entirely sure whether we should backpatch this (either in it's entirety, or some subset of the changes). I believe it's harmless, as it can only slow down cases where the behavior is likely incorrect. OTOH I don't recall any reports about issues with this, so chances are people are not using it with ROW triggers etc. (But then it also should not get slower for them.) regards [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=1e0a55a0f -- Tomas Vondra
From f5f032bc13877fc6ddce4a56cd0a1723bab273b5 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Thu, 12 Nov 2026 19:32:58 +0100 Subject: [PATCH v1 3/4] Allow batching with OLD TABLE transition tables Tuples routed to a foreign partition are not batched when the statement captures transition tuples, so that the per-row trigger code can reject collecting transition tuples from foreign partitions. But routed tuples can only be captured in a NEW TABLE, whether they come from an INSERT or from an UPDATE moving rows between partitions. An OLD TABLE never contains them, as the old version of a row moved by a cross-partition UPDATE is captured when it's deleted from the source partition. So disable batching only if the routed tuples may have to be captured in a NEW TABLE. This only matters for UPDATE and MERGE (INSERT and COPY FROM capture transition tuples only for a NEW TABLE). A cross-partition UPDATE on a partitioned table with only OLD TABLE transition tables batches inserts into foreign partitions again. So does MERGE with only OLD TABLE transition tables. MERGE prepares transition capture for all kinds of actions, so a NEW TABLE for UPDATE still disables batching even for tuples routed to a foreign partition by an INSERT action. The regression test with an OLD TABLE now expects a single batched INSERT for the moved rows, and a new test checks that MERGE with only an OLD TABLE (for DELETE) batches the tuples routed by an INSERT action. --- .../postgres_fdw/expected/postgres_fdw.out | 26 +++++++++++++++---- contrib/postgres_fdw/sql/postgres_fdw.sql | 19 +++++++++++--- doc/src/sgml/fdwhandler.sgml | 10 ++++--- src/backend/executor/execMain.c | 19 +++++++++----- 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 35def829d31..a3598ac576e 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -12276,9 +12276,10 @@ SELECT * FROM cmdlog ORDER BY 1; (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. +-- Transition tables on the parent disable batching only if the routed rows +-- may have to be captured in a NEW TABLE. An OLD TABLE never contains them +-- (a moved row is captured when deleting it from the source partition), so +-- expect a single INSERT on batch_cp_upd_test1 for both moved rows. CREATE TRIGGER oldtab_trig AFTER UPDATE ON batch_cp_upd_test REFERENCING OLD TABLE AS oldtab FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); @@ -12288,10 +12289,9 @@ 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) +(2 rows) DROP TRIGGER oldtab_trig ON batch_cp_upd_test; -- With a NEW TABLE, batching is disabled, and the per-row code refuses to @@ -12318,6 +12318,22 @@ SELECT * FROM cmdlog ORDER BY 1; (2 rows) DROP TRIGGER newtab_trig ON batch_cp_upd_test; +-- As with UPDATE, an OLD TABLE doesn't disable batching for MERGE: with only +-- a DELETE OLD TABLE, expect a single INSERT on batch_cp_upd_test1. +CREATE TRIGGER oldtab_del_trig AFTER DELETE ON batch_cp_upd_test + REFERENCING OLD TABLE AS oldtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +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 +(1 row) + +DROP TRIGGER oldtab_del_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/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 3ee739d40ef..58e268be8c2 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -4258,9 +4258,10 @@ 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. +-- Transition tables on the parent disable batching only if the routed rows +-- may have to be captured in a NEW TABLE. An OLD TABLE never contains them +-- (a moved row is captured when deleting it from the source partition), so +-- expect a single INSERT on batch_cp_upd_test1 for both moved rows. CREATE TRIGGER oldtab_trig AFTER UPDATE ON batch_cp_upd_test REFERENCING OLD TABLE AS oldtab FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); @@ -4289,6 +4290,18 @@ MERGE INTO batch_cp_upd_test t USING (VALUES (1), (1)) s(a) SELECT * FROM cmdlog ORDER BY 1; DROP TRIGGER newtab_trig ON batch_cp_upd_test; +-- As with UPDATE, an OLD TABLE doesn't disable batching for MERGE: with only +-- a DELETE OLD TABLE, expect a single INSERT on batch_cp_upd_test1. +CREATE TRIGGER oldtab_del_trig AFTER DELETE ON batch_cp_upd_test + REFERENCING OLD TABLE AS oldtab + FOR EACH STATEMENT EXECUTE FUNCTION log_stmt(); +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 oldtab_del_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 ea288520dfc..2b35b6d475c 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -675,10 +675,12 @@ ExecForeignBatchInsert(EState *estate, the statement has a <literal>RETURNING</literal> clause, there are <literal>WITH CHECK OPTION</literal> constraints to check, or the foreign table has <literal>BEFORE ROW</literal> or <literal>AFTER ROW</literal> - <command>INSERT</command> 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. + <command>INSERT</command> triggers. Batching is also disabled for tuples + routed to a foreign-table partition when they may have to be captured in + a <literal>NEW TABLE</literal> transition relation of a parent table. + Statement-level triggers without transition tables, or with only + <literal>OLD TABLE</literal> transition relations, do not prevent + batching. </para> <para> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 55bd62dd313..3a469a85b0a 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1380,11 +1380,16 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, * transition_capture is the statement's transition capture state when routing * tuples to a partition, or NULL when inserting into the result relation * directly, because a foreign table can't have transition tables of its own. - * - * FIXME The transition_capture check 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? + * Don't batch if the routed tuples may have to be added to a NEW TABLE (when + * inserting, or when an UPDATE moves rows to another partition). Transition + * tuples can't be collected from foreign partitions, so the per-row code + * (ExecARInsertTriggers() and ExecARUpdateTriggers()) raises an error, while + * batching would silently leave the tuples out of the NEW TABLE. An OLD + * TABLE never contains routed tuples (the old version of a moved row is + * captured when it's deleted from the source partition), so it doesn't + * prevent batching. MERGE sets up transition capture for all kinds of + * actions, so an UPDATE NEW TABLE disables batching even for tuples routed by + * an INSERT action. * * The FDW's GetForeignModifyBatchSize callback is called only if batching is * possible; otherwise the batch size is 1. @@ -1406,7 +1411,9 @@ ExecInitInsertBatchSize(ResultRelInfo *resultRelInfo, (trigdesc == NULL || (!trigdesc->trig_insert_before_row && !trigdesc->trig_insert_after_row)) && - transition_capture == NULL) + (transition_capture == NULL || + (!transition_capture->tcs_insert_new_table && + !transition_capture->tcs_update_new_table))) { batch_size = fdwroutine->GetForeignModifyBatchSize(resultRelInfo); -- 2.56.0
From 351916be71b7a816e3ade13a72b38dfb39b54454 Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> Date: Thu, 12 Nov 2026 19:29:28 +0100 Subject: [PATCH v1 2/4] Move the batching decision for foreign tables to a single function The previous commit moved the decision whether inserts into a foreign table can be batched from the FDW to the core code, but it copied the same check in three places. The copies already differed in small ways. Replace the three copies, along with the FIXME comments about them, with a new function ExecInitInsertBatchSize(). Callers pass the statement's transition capture state only when routing tuples, as a foreign table can't have transition tables of its own. The function clamps the batch size returned by the FDW to at least 1 in all three cases, as COPY did. An FDW returning a value below 1 for INSERT or tuple routing used to fail an assertion in assert-enabled builds. Such inserts are now done row by row, as they already were in builds without assertions. Tuple routing now also checks ri_usesFdwDirectModify, as INSERT did. This only matters when routing tuples into a foreign table that the same UPDATE modifies directly, which postgres_fdw rejects anyway. --- src/backend/commands/copyfrom.c | 37 +---------------- src/backend/executor/execMain.c | 56 ++++++++++++++++++++++++++ src/backend/executor/execPartition.c | 47 +++------------------ src/backend/executor/nodeModifyTable.c | 35 +--------------- src/include/executor/executor.h | 2 + 5 files changed, 66 insertions(+), 111 deletions(-) diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c index b75625e65ff..7048c9663ca 100644 --- a/src/backend/commands/copyfrom.c +++ b/src/backend/commands/copyfrom.c @@ -786,7 +786,6 @@ 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); @@ -928,46 +927,12 @@ 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 * batching, but it may be disabled for the server/table). - * - * If the FDW does not support batching, we set the batch size to 1. - */ - if (resultRelInfo->ri_FdwRoutine != NULL && - resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && - resultRelInfo->ri_FdwRoutine->ExecForeignBatchInsert && - allow_batching) - resultRelInfo->ri_BatchSize = - resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo); - else - 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); + ExecInitInsertBatchSize(resultRelInfo, NULL); /* Prepare to catch AFTER triggers. */ AfterTriggerBeginQuery(); diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index cfe1c2a801b..55bd62dd313 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1364,6 +1364,62 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo, resultRelInfo->ri_CopyMultiInsertBuffer = NULL; } +/* + * ExecInitInsertBatchSize + * Decide whether inserts into a foreign table can be batched, and set + * the batch size of the result relation accordingly. + * + * ExecBatchInsert() and CopyMultiInsertBufferFlush() insert batches without + * any of the per-row processing done after inserting a single row: they don't + * fire AFTER ROW triggers, check WITH CHECK OPTIONs, project RETURNING, or + * capture transition tuples. So we batch only when none of that is needed. + * BEFORE ROW triggers prevent batching too. The triggers might query the + * target table, so the pending inserts would have to be flushed before firing + * them for each row, which makes batching pointless. + * + * transition_capture is the statement's transition capture state when routing + * tuples to a partition, or NULL when inserting into the result relation + * directly, because a foreign table can't have transition tables of its own. + * + * FIXME The transition_capture check 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? + * + * The FDW's GetForeignModifyBatchSize callback is called only if batching is + * possible; otherwise the batch size is 1. + */ +void +ExecInitInsertBatchSize(ResultRelInfo *resultRelInfo, + TransitionCaptureState *transition_capture) +{ + FdwRoutine *fdwroutine = resultRelInfo->ri_FdwRoutine; + TriggerDesc *trigdesc = resultRelInfo->ri_TrigDesc; + int batch_size = 1; + + if (fdwroutine != NULL && + !resultRelInfo->ri_usesFdwDirectModify && + fdwroutine->GetForeignModifyBatchSize != NULL && + fdwroutine->ExecForeignBatchInsert != NULL && + resultRelInfo->ri_projectReturning == NULL && + resultRelInfo->ri_WithCheckOptions == NIL && + (trigdesc == NULL || + (!trigdesc->trig_insert_before_row && + !trigdesc->trig_insert_after_row)) && + transition_capture == NULL) + { + batch_size = fdwroutine->GetForeignModifyBatchSize(resultRelInfo); + + /* + * Don't rely on the FDW to return a valid batch size. With a batch + * size below 1, CopyMultiInsertBufferFlush() would loop forever. + */ + batch_size = Max(batch_size, 1); + } + + resultRelInfo->ri_BatchSize = batch_size; +} + /* * ExecGetTriggerResultRel * Get a ResultRelInfo for a trigger target relation. diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c index cd0d20aa3c1..79e45193db5 100644 --- a/src/backend/executor/execPartition.c +++ b/src/backend/executor/execPartition.c @@ -1113,7 +1113,6 @@ ExecInitRoutingInfo(ModifyTableState *mtstate, { MemoryContext oldcxt; int rri_index; - bool allow_batching = true; oldcxt = MemoryContextSwitchTo(proute->memcxt); @@ -1146,52 +1145,18 @@ 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 * server/table or for this particular query). * - * If the FDW does not support batching, we set the batch size to 1. + * partRelInfo has the partition's WITH CHECK OPTIONs and RETURNING list + * set up already (by ExecInitPartitionInfo(), or by ExecInitModifyTable() + * for a reused UPDATE result relation), and its triggers include the row + * triggers cloned from partitioned ancestors. Transition tables belong + * to the statement's target table, so pass its transition capture state. */ - if (partRelInfo->ri_FdwRoutine != NULL && - partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize && - partRelInfo->ri_FdwRoutine->ExecForeignBatchInsert && - allow_batching) - partRelInfo->ri_BatchSize = - partRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(partRelInfo); - else - partRelInfo->ri_BatchSize = 1; - - Assert(partRelInfo->ri_BatchSize >= 1); + ExecInitInsertBatchSize(partRelInfo, mtstate->mt_transition_capture); partRelInfo->ri_CopyMultiInsertBuffer = NULL; diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index a78aaaea6dd..0c61b854d47 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -5457,42 +5457,9 @@ 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 && - allow_batching) - { - resultRelInfo->ri_BatchSize = - resultRelInfo->ri_FdwRoutine->GetForeignModifyBatchSize(resultRelInfo); - Assert(resultRelInfo->ri_BatchSize >= 1); - } - else - resultRelInfo->ri_BatchSize = 1; + ExecInitInsertBatchSize(mtstate->resultRelInfo, NULL); } /* diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index b126e629315..ba763560778 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -255,6 +255,8 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo, Index resultRelationIndex, ResultRelInfo *partition_root_rri, int instrument_options); +extern void ExecInitInsertBatchSize(ResultRelInfo *resultRelInfo, + struct TransitionCaptureState *transition_capture); extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid, ResultRelInfo *rootRelInfo); extern List *ExecGetAncestorResultRels(EState *estate, ResultRelInfo *resultRelInfo); -- 2.56.0
From 21480f1c6b858877e45919d551dc47a85bb2a89f Mon Sep 17 00:00:00 2001 From: Tomas Vondra <[email protected]> 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, </para> <para> - 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 <literal>slots</literal> can be re-used for this purpose. - The number of successfully inserted tuples is returned in - <literal>*numSlots</literal>. + The function must set <literal>*numSlots</literal> to the number of + successfully inserted tuples. This can be fewer than the number of + tuples passed in, or even zero, for example when + <literal>ON CONFLICT DO NOTHING</literal> skips conflicting tuples. + The executor ignores the returned slot array, so the function can return + <literal>NULL</literal>. It need not populate slots with the data that + was actually inserted. </para> <para> - The data in the returned slot is used only if the <command>INSERT</command> - statement involves a view - <literal>WITH CHECK OPTION</literal>; or if the foreign table has - an <literal>AFTER ROW</literal> trigger. Triggers require all columns, - but the FDW could choose to optimize away returning some or all columns - depending on the contents of the - <literal>WITH CHECK OPTION</literal> constraints. + The executor uses <function>ExecForeignInsert</function> instead when + the statement has a <literal>RETURNING</literal> clause, there are + <literal>WITH CHECK OPTION</literal> constraints to check, or the foreign + table has <literal>BEFORE ROW</literal> or <literal>AFTER ROW</literal> + <command>INSERT</command> 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. </para> <para> @@ -684,8 +686,6 @@ ExecForeignBatchInsert(EState *estate, <function>GetForeignModifyBatchSize</function> pointer is set to <literal>NULL</literal>, attempts to insert into the foreign table will use <function>ExecForeignInsert</function>. - This function is not used if the <command>INSERT</command> has the - <literal>RETURNING</literal> clause. </para> <para> @@ -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. + </para> + + <para> + This function is not called when the executor has disabled batching for + one of the reasons described for + <function>ExecForeignBatchInsert</function>. </para> <para> 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
