On 9/8/20 8:34 PM, Alexey Kondratov wrote:
On 2020-09-08 17:00, Amit Langote wrote:
<a.kondra...@postgrespro.ru> wrote:
On 2020-09-08 10:34, Amit Langote wrote:
Another ambiguous part of the refactoring was in changing
InitResultRelInfo() arguments:
@@ -1278,6 +1280,7 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
Relation partition_root,
+ bool use_multi_insert,
int instrument_options)
Why do we need to pass this use_multi_insert flag here? Would it be
better to set resultRelInfo->ri_usesMultiInsert in the
InitResultRelInfo() unconditionally like it is done for
ri_usesFdwDirectModify? And after that it will be up to the caller
whether to use multi-insert or not based on their own circumstances.
Otherwise now we have a flag to indicate that we want to check for
another flag, while this check doesn't look costly.
Hmm, I think having two flags seems confusing and bug prone,
especially if you consider partitions. For example, if a partition's
ri_usesMultiInsert is true, but CopyFrom()'s local flag is false, then
execPartition.c: ExecInitPartitionInfo() would wrongly perform
BeginForeignCopy() based on only ri_usesMultiInsert, because it
wouldn't know CopyFrom()'s local flag. Am I missing something?
No, you're right. If someone want to share a state and use ResultRelInfo
(RRI) for that purpose, then it's fine, but CopyFrom() may simply
override RRI->ri_usesMultiInsert if needed and pass this RRI further.
This is how it's done for RRI->ri_usesFdwDirectModify.
InitResultRelInfo() initializes it to false and then
ExecInitModifyTable() changes the flag if needed.
Probably this is just a matter of personal choice, but for me the
current implementation with additional argument in InitResultRelInfo()
doesn't look completely right. Maybe because a caller now should pass an
additional argument (as false) even if it doesn't care about
ri_usesMultiInsert at all. It also adds additional complexity and feels
like abstractions leaking.
I didn't feel what the problem was and prepared a patch version
according to Alexey's suggestion (see Alternate.patch).
This does not seem very convenient and will lead to errors in the
future. So, I agree with Amit.
--
regards,
Andrey Lepikhov
Postgres Professional
>From 73705843d300ad1016384e6cb8893c80246372a6 Mon Sep 17 00:00:00 2001
From: amitlan <amitlangot...@gmail.com>
Date: Mon, 24 Aug 2020 15:08:37 +0900
Subject: [PATCH 1/2] Move multi-insert decision logic into executor
When 0d5f05cde introduced support for using multi-insert mode when
copying into partitioned tables, it introduced single variable of
enum type CopyInsertMethod shared across all potential target
relations (partitions) that, along with some target relation
proprties, dictated whether to engage multi-insert mode for a given
target relation.
Move that decision logic into InitResultRelInfo which now sets a new
boolean field ri_usesMultiInsert of ResultRelInfo when a target
relation is first initialized. That prevents repeated computation
of the same information in some cases, especially for partitions,
and the new arrangement results in slightly more readability.
---
src/backend/commands/copy.c | 189 +++++++++--------------
src/backend/commands/tablecmds.c | 1 +
src/backend/executor/execMain.c | 40 +++++
src/backend/executor/execPartition.c | 7 +
src/backend/replication/logical/worker.c | 1 +
src/include/nodes/execnodes.h | 9 +-
6 files changed, 127 insertions(+), 120 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index db7d24a511..94f6e71a94 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -85,16 +85,6 @@ typedef enum EolType
EOL_CRNL
} EolType;
-/*
- * Represents the heap insert method to be used during COPY FROM.
- */
-typedef enum CopyInsertMethod
-{
- CIM_SINGLE, /* use table_tuple_insert or fdw routine */
- CIM_MULTI, /* always use table_multi_insert */
- CIM_MULTI_CONDITIONAL /* use table_multi_insert only if valid */
-} CopyInsertMethod;
-
/*
* This struct contains all the state variables used throughout a COPY
* operation. For simplicity, we use the same struct for all variants of COPY,
@@ -2715,12 +2705,10 @@ CopyFrom(CopyState cstate)
CommandId mycid = GetCurrentCommandId(true);
int ti_options = 0; /* start with default options for insert */
BulkInsertState bistate = NULL;
- CopyInsertMethod insertMethod;
CopyMultiInsertInfo multiInsertInfo = {0}; /* pacify compiler */
uint64 processed = 0;
bool has_before_insert_row_trig;
bool has_instead_insert_row_trig;
- bool leafpart_use_multi_insert = false;
Assert(cstate->rel);
@@ -2833,6 +2821,57 @@ CopyFrom(CopyState cstate)
0);
target_resultRelInfo = resultRelInfo;
+ /*
+ * It's generally more efficient to prepare a bunch of tuples for
+ * insertion, and insert them in bulk, for example, with one
+ * table_multi_insert() call than call table_tuple_insert() separately
+ * for every tuple. However, there are a number of reasons why we might
+ * not be able to do this. We check some conditions below while some
+ * other target relation properties are checked in InitResultRelInfo().
+ * Partition initialization will use result of this check implicitly as
+ * the ri_usesMultiInsert value of the parent relation.
+ */
+ if (!target_resultRelInfo->ri_usesMultiInsert)
+ {
+ /*
+ * Do nothing. Can't allow multi-insert mode if previous conditions
+ * checking in the InitResultRelInfo() routine disallow this.
+ */
+ }
+ else if (cstate->volatile_defexprs || list_length(cstate->attnumlist) == 0)
+ {
+ /*
+ * Can't support bufferization of copy into foreign tables without any
+ * defined columns or if there are any volatile default expressions in the
+ * table. Similarly to the trigger case above, such expressions may query
+ * the table we're inserting into.
+ *
+ * Note: It does not matter if any partitions have any volatile
+ * default expressions as we use the defaults from the target of the
+ * COPY command.
+ */
+ target_resultRelInfo->ri_usesMultiInsert = false;
+ }
+ else if (contain_volatile_functions(cstate->whereClause))
+ {
+ /*
+ * Can't support multi-inserts if there are any volatile function
+ * expressions in WHERE clause. Similarly to the trigger case above,
+ * such expressions may query the table we're inserting into.
+ */
+ target_resultRelInfo->ri_usesMultiInsert = false;
+ }
+ else
+ {
+ /*
+ * Looks okay to try multi-insert.
+ *
+ * For partitioned tables, whether or not to use multi-insert depends
+ * on the individual parition's properties which are also checked in
+ * InitResultRelInfo().
+ */
+ }
+
/* Verify the named relation is a valid target for INSERT */
CheckValidResultRel(resultRelInfo, CMD_INSERT);
@@ -2854,10 +2893,14 @@ CopyFrom(CopyState cstate)
mtstate->operation = CMD_INSERT;
mtstate->resultRelInfo = estate->es_result_relations;
- if (resultRelInfo->ri_FdwRoutine != NULL &&
- resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
- resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
- resultRelInfo);
+ /*
+ * Init COPY into foreign table. Initialization of copying into foreign
+ * partitions will be done later.
+ */
+ if (target_resultRelInfo->ri_FdwRoutine != NULL &&
+ target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
+ target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
+ resultRelInfo);
/* Prepare to catch AFTER triggers. */
AfterTriggerBeginQuery();
@@ -2886,83 +2929,9 @@ CopyFrom(CopyState cstate)
cstate->qualexpr = ExecInitQual(castNode(List, cstate->whereClause),
&mtstate->ps);
- /*
- * It's generally more efficient to prepare a bunch of tuples for
- * insertion, and insert them in one table_multi_insert() call, than call
- * table_tuple_insert() separately for every tuple. However, there are a
- * number of reasons why we might not be able to do this. These are
- * explained below.
- */
- if (resultRelInfo->ri_TrigDesc != NULL &&
- (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
- resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
- {
- /*
- * Can't support multi-inserts when there are any BEFORE/INSTEAD OF
- * triggers on the table. 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.
- */
- insertMethod = CIM_SINGLE;
- }
- else if (proute != NULL && resultRelInfo->ri_TrigDesc != NULL &&
- resultRelInfo->ri_TrigDesc->trig_insert_new_table)
- {
- /*
- * For partitioned tables we can't support multi-inserts when there
- * are any statement level insert triggers. It might be possible to
- * allow partitioned tables with such triggers in the future, but for
- * now, CopyMultiInsertInfoFlush expects that any before row insert
- * and statement level insert triggers are on the same relation.
- */
- insertMethod = CIM_SINGLE;
- }
- else if (resultRelInfo->ri_FdwRoutine != NULL ||
- cstate->volatile_defexprs)
- {
- /*
- * Can't support multi-inserts to foreign tables or if there are any
- * volatile default expressions in the table. Similarly to the
- * trigger case above, such expressions may query the table we're
- * inserting into.
- *
- * Note: It does not matter if any partitions have any volatile
- * default expressions as we use the defaults from the target of the
- * COPY command.
- */
- insertMethod = CIM_SINGLE;
- }
- else if (contain_volatile_functions(cstate->whereClause))
- {
- /*
- * Can't support multi-inserts if there are any volatile function
- * expressions in WHERE clause. Similarly to the trigger case above,
- * such expressions may query the table we're inserting into.
- */
- insertMethod = CIM_SINGLE;
- }
- else
- {
- /*
- * For partitioned tables, we may still be able to perform bulk
- * inserts. However, the possibility of this depends on which types
- * of triggers exist on the partition. We must disable bulk inserts
- * if the partition is a foreign table or it has any before row insert
- * or insert instead triggers (same as we checked above for the parent
- * table). Since the partition's resultRelInfos are initialized only
- * when we actually need to insert the first tuple into them, we must
- * have the intermediate insert method of CIM_MULTI_CONDITIONAL to
- * flag that we must later determine if we can use bulk-inserts for
- * the partition being inserted into.
- */
- if (proute)
- insertMethod = CIM_MULTI_CONDITIONAL;
- else
- insertMethod = CIM_MULTI;
-
+ if (resultRelInfo->ri_usesMultiInsert)
CopyMultiInsertInfoInit(&multiInsertInfo, resultRelInfo, cstate,
estate, mycid, ti_options);
- }
/*
* If not using batch mode (which allocates slots as needed) set up a
@@ -2970,7 +2939,7 @@ CopyFrom(CopyState cstate)
* one, even if we might batch insert, to read the tuple in the root
* partition's form.
*/
- if (insertMethod == CIM_SINGLE || insertMethod == CIM_MULTI_CONDITIONAL)
+ if (!resultRelInfo->ri_usesMultiInsert || proute)
{
singleslot = table_slot_create(resultRelInfo->ri_RelationDesc,
&estate->es_tupleTable);
@@ -3013,7 +2982,7 @@ CopyFrom(CopyState cstate)
ResetPerTupleExprContext(estate);
/* select slot to (initially) load row into */
- if (insertMethod == CIM_SINGLE || proute)
+ if (!target_resultRelInfo->ri_usesMultiInsert || proute)
{
myslot = singleslot;
Assert(myslot != NULL);
@@ -3021,7 +2990,6 @@ CopyFrom(CopyState cstate)
else
{
Assert(resultRelInfo == target_resultRelInfo);
- Assert(insertMethod == CIM_MULTI);
myslot = CopyMultiInsertInfoNextFreeSlot(&multiInsertInfo,
resultRelInfo);
@@ -3080,24 +3048,14 @@ CopyFrom(CopyState cstate)
has_instead_insert_row_trig = (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_insert_instead_row);
- /*
- * Disable multi-inserts when the partition has BEFORE/INSTEAD
- * OF triggers, or if the partition is a foreign partition.
- */
- leafpart_use_multi_insert = insertMethod == CIM_MULTI_CONDITIONAL &&
- !has_before_insert_row_trig &&
- !has_instead_insert_row_trig &&
- resultRelInfo->ri_FdwRoutine == NULL;
-
/* Set the multi-insert buffer to use for this partition. */
- if (leafpart_use_multi_insert)
+ if (resultRelInfo->ri_usesMultiInsert)
{
if (resultRelInfo->ri_CopyMultiInsertBuffer == NULL)
CopyMultiInsertInfoSetupBuffer(&multiInsertInfo,
resultRelInfo);
}
- else if (insertMethod == CIM_MULTI_CONDITIONAL &&
- !CopyMultiInsertInfoIsEmpty(&multiInsertInfo))
+ else if (!CopyMultiInsertInfoIsEmpty(&multiInsertInfo))
{
/*
* Flush pending inserts if this partition can't use
@@ -3149,7 +3107,7 @@ CopyFrom(CopyState cstate)
* rowtype.
*/
map = resultRelInfo->ri_PartitionInfo->pi_RootToPartitionMap;
- if (insertMethod == CIM_SINGLE || !leafpart_use_multi_insert)
+ if (!resultRelInfo->ri_usesMultiInsert)
{
/* non batch insert */
if (map != NULL)
@@ -3168,9 +3126,6 @@ CopyFrom(CopyState cstate)
*/
TupleTableSlot *batchslot;
- /* no other path available for partitioned table */
- Assert(insertMethod == CIM_MULTI_CONDITIONAL);
-
batchslot = CopyMultiInsertInfoNextFreeSlot(&multiInsertInfo,
resultRelInfo);
@@ -3241,7 +3196,7 @@ CopyFrom(CopyState cstate)
ExecPartitionCheck(resultRelInfo, myslot, estate, true);
/* Store the slot in the multi-insert buffer, when enabled. */
- if (insertMethod == CIM_MULTI || leafpart_use_multi_insert)
+ if (resultRelInfo->ri_usesMultiInsert)
{
/*
* The slot previously might point into the per-tuple
@@ -3316,11 +3271,8 @@ CopyFrom(CopyState cstate)
}
/* Flush any remaining buffered tuples */
- if (insertMethod != CIM_SINGLE)
- {
- if (!CopyMultiInsertInfoIsEmpty(&multiInsertInfo))
- CopyMultiInsertInfoFlush(&multiInsertInfo, NULL);
- }
+ if (!CopyMultiInsertInfoIsEmpty(&multiInsertInfo))
+ CopyMultiInsertInfoFlush(&multiInsertInfo, NULL);
/* Done, clean up */
error_context_stack = errcallback.previous;
@@ -3349,11 +3301,10 @@ CopyFrom(CopyState cstate)
if (target_resultRelInfo->ri_FdwRoutine != NULL &&
target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate,
- target_resultRelInfo);
+ target_resultRelInfo);
/* Tear down the multi-insert buffer data */
- if (insertMethod != CIM_SINGLE)
- CopyMultiInsertInfoCleanup(&multiInsertInfo);
+ CopyMultiInsertInfoCleanup(&multiInsertInfo);
ExecCloseIndices(target_resultRelInfo);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3e57c7f9e1..571f209429 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1800,6 +1800,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
0, /* dummy rangetable index */
NULL,
0);
+ resultRelInfo->ri_usesMultiInsert = false;
resultRelInfo++;
}
estate->es_result_relations = resultRelInfos;
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 4fdffad6f3..11ae3e1a82 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -852,6 +852,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelationIndex,
NULL,
estate->es_instrument);
+ resultRelInfo->ri_usesMultiInsert = false;
resultRelInfo++;
}
estate->es_result_relations = resultRelInfos;
@@ -884,6 +885,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
resultRelIndex,
NULL,
estate->es_instrument);
+ resultRelInfo->ri_usesMultiInsert = false;
resultRelInfo++;
}
@@ -1345,6 +1347,43 @@ InitResultRelInfo(ResultRelInfo *resultRelInfo,
resultRelInfo->ri_PartitionRoot = partition_root;
resultRelInfo->ri_PartitionInfo = NULL; /* may be set later */
resultRelInfo->ri_CopyMultiInsertBuffer = NULL;
+
+ /* Check if the relation allows to use "multi-insert" mode. */
+ if (resultRelInfo->ri_TrigDesc != NULL &&
+ (resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
+ resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
+ {
+ /*
+ * Can't support multi-inserts when there are any BEFORE/INSTEAD OF
+ * triggers on the table. 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.
+ */
+ resultRelInfo->ri_usesMultiInsert = false;
+ }
+ else if (resultRelationDesc->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
+ resultRelInfo->ri_TrigDesc != NULL &&
+ resultRelInfo->ri_TrigDesc->trig_insert_new_table)
+ {
+ /*
+ * For partitioned tables we can't support multi-inserts when there
+ * are any statement level insert triggers. It might be possible to
+ * allow partitioned tables with such triggers in the future, but for
+ * now, CopyMultiInsertInfoFlush expects that any before row insert
+ * and statement level insert triggers are on the same relation.
+ */
+ resultRelInfo->ri_usesMultiInsert = false;
+ }
+ else if (resultRelInfo->ri_FdwRoutine != NULL)
+ {
+ /* Foreign tables don't support multi-inserts. */
+ resultRelInfo->ri_usesMultiInsert = false;
+ }
+ else
+ {
+ /* OK, caller can use multi-insert on this relation. */
+ resultRelInfo->ri_usesMultiInsert = true;
+ }
}
/*
@@ -1435,6 +1474,7 @@ ExecGetTriggerResultRel(EState *estate, Oid relid)
0, /* dummy rangetable index */
NULL,
estate->es_instrument);
+ rInfo->ri_usesMultiInsert = false;
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
MemoryContextSwitchTo(oldcontext);
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index bd2ea25804..db54107caa 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -583,6 +583,13 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
rootrel,
estate->es_instrument);
+ /*
+ * Use multi-insert mode if the initial condition checking passes for the
+ * parent and its child.
+ */
+ leaf_part_rri->ri_usesMultiInsert = (leaf_part_rri->ri_usesMultiInsert &&
+ rootResultRelInfo->ri_usesMultiInsert) ? true : false;
+
/*
* Verify result relation is a valid target for an INSERT. An UPDATE of a
* partition-key becomes a DELETE+INSERT operation, so this check is still
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index c37aafed0d..9858aca6ef 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -358,6 +358,7 @@ create_estate_for_relation(LogicalRepRelMapEntry *rel)
resultRelInfo = makeNode(ResultRelInfo);
InitResultRelInfo(resultRelInfo, rel->localrel, 1, NULL, 0);
+ resultRelInfo->ri_usesMultiInsert = false;
estate->es_result_relations = resultRelInfo;
estate->es_num_result_relations = 1;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 0b42dd6f94..89ae9afaa4 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -489,7 +489,14 @@ typedef struct ResultRelInfo
/* Additional information specific to partition tuple routing */
struct PartitionRoutingInfo *ri_PartitionInfo;
- /* For use by copy.c when performing multi-inserts */
+ /*
+ * The following fields are currently only relevant to copy.c.
+ *
+ * True if okay to use multi-insert on this relation
+ */
+ bool ri_usesMultiInsert;
+
+ /* Buffer allocated to this relation when using multi-insert mode */
struct CopyMultiInsertBuffer *ri_CopyMultiInsertBuffer;
} ResultRelInfo;
--
2.25.1
