From 351916be71b7a816e3ade13a72b38dfb39b54454 Mon Sep 17 00:00:00 2001 From: Tomas Vondra 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