Thanks for the review.
On 2018/08/17 15:00, Amit Khandekar wrote:
> Thanks for the patch. I did some review of the patch (needs a rebase).
> Below are my comments.
>
> @@ -1936,12 +1936,11 @@ ExecPartitionCheckEmitError(ResultRelInfo
> *resultRelInfo,
> + /* Input slot might be of a partition, which has a fixed tupdesc. */
> + slot = MakeTupleTableSlot(tupdesc);
> if (map != NULL)
> - {
> tuple = do_convert_tuple(tuple, map);
> - ExecSetSlotDescriptor(slot, tupdesc);
> - ExecStoreTuple(tuple, slot, InvalidBuffer, false);
> - }
> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>
> Both MakeTupleTableSlot() and ExecStoreTuple() can be inside the (map
> != NULL) if condition.
> This also applies for similar changes in ExecConstraints() and
> ExecWithCheckOptions().
Ah, okay. I guess that means we'll allocate a new slot here only if we
had to switch to a partition-specific slot in the first place.
> + * Initialize an empty slot that will be used to manipulate tuples of any
> + * this partition's rowtype.
> of any this => of this
>
> + * Initialized the array where these slots are stored, if not already
> Initialized => Initialize
Fixed.
> + proute->partition_tuple_slots_alloced =
> + lappend(proute->partition_tuple_slots_alloced,
> + proute->partition_tuple_slots[partidx]);
>
> Instead of doing the above, I think we can collect those slots in
> estate->es_tupleTable using ExecInitExtraTupleSlot() so that they
> don't have to be explicitly dropped in ExecCleanupTupleRouting(). And
> also then we won't require the new field
> proute->partition_tuple_slots_alloced.
Although I was slightly uncomfortable of the idea at first, thinking that
it's not good for tuple routing specific resources to be released by
generic executor code, doesn't seem too bad to do it the way you suggest.
Attached updated patch. By the way, I think it might be a good idea to
try to merge this patch with the effort in the following thread.
* Reduce partition tuple routing overheads *
https://commitfest.postgresql.org/19/1690/
Thanks,
Amit
From a543b15f83f5e1adaef2a46de59022b0b21711e4 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Thu, 28 Jun 2018 15:47:47 +0900
Subject: [PATCH v2] Allocate dedicated slots of partition tuple conversion
Currently there is just one slot called partition_tuple_slot and
we do ExecSetSlotDescriptor every time a tuple is inserted into a
partition that requires tuple conversion. That's excessively
inefficient during bulk inserts.
Fix that by allocating a dedicated slot for each partitions that
requires it.
---
src/backend/commands/copy.c | 17 +++++++++----
src/backend/executor/execMain.c | 24 +++++++++++++++---
src/backend/executor/execPartition.c | 45 +++++++++++++++++++++++-----------
src/backend/executor/nodeModifyTable.c | 27 ++++++++++++--------
src/include/executor/execPartition.h | 13 ++++++----
5 files changed, 88 insertions(+), 38 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 9bc67ce60f..f61db3e173 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2690,6 +2690,7 @@ CopyFrom(CopyState cstate)
if (proute)
{
int leaf_part_index;
+ TupleConversionMap *map;
/*
* Away we go ... If we end up not finding a partition
after all,
@@ -2864,11 +2865,17 @@ CopyFrom(CopyState cstate)
* partition rowtype. Don't free the already stored
tuple as it
* may still be required for a multi-insert batch.
*/
- tuple =
ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[leaf_part_index],
-
tuple,
-
proute->partition_tuple_slot,
-
&slot,
-
false);
+ map =
proute->parent_child_tupconv_maps[leaf_part_index];
+ if (map != NULL)
+ {
+ TupleTableSlot *new_slot;
+
+ Assert(proute->partition_tuple_slots != NULL &&
+
proute->partition_tuple_slots[leaf_part_index] != NULL);
+ new_slot =
proute->partition_tuple_slots[leaf_part_index];
+ tuple = ConvertPartitionTupleSlot(map, tuple,
new_slot, &slot,
+
false);
+ }
tuple->t_tableOid =
RelationGetRelid(resultRelInfo->ri_RelationDesc);
}
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index c583e020a0..415670ed7e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1938,8 +1938,12 @@ ExecPartitionCheckEmitError(ResultRelInfo *resultRelInfo,
gettext_noop("could not convert row type"));
if (map != NULL)
{
+ /*
+ * Partition-specific slot's tupdesc can't be changed,
so allocate
+ * a new one.
+ */
+ slot = MakeTupleTableSlot(tupdesc);
tuple = do_convert_tuple(tuple, map);
- ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, InvalidBuffer, false);
}
}
@@ -2017,8 +2021,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
gettext_noop("could not convert row type"));
if (map != NULL)
{
+ /*
+ * Partition-specific slot's
tupdesc can't be changed,
+ * so allocate a new one.
+ */
+ slot =
MakeTupleTableSlot(tupdesc);
tuple = do_convert_tuple(tuple,
map);
- ExecSetSlotDescriptor(slot,
tupdesc);
ExecStoreTuple(tuple, slot,
InvalidBuffer, false);
}
}
@@ -2065,8 +2073,12 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
gettext_noop("could not convert row type"));
if (map != NULL)
{
+ /*
+ * Partition-specific slot's tupdesc
can't be changed,
+ * so allocate a new one.
+ */
+ slot = MakeTupleTableSlot(tupdesc);
tuple = do_convert_tuple(tuple, map);
- ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot,
InvalidBuffer, false);
}
}
@@ -2171,8 +2183,12 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo
*resultRelInfo,
gettext_noop("could not convert row type"));
if (map != NULL)
{
+ /*
+ * Partition-specific
slot's tupdesc can't be
+ * changed, so allocate
a new one.
+ */
+ slot =
MakeTupleTableSlot(tupdesc);
tuple =
do_convert_tuple(tuple, map);
-
ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple,
slot, InvalidBuffer, false);
}
}
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 1a9943c3aa..2af7599152 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -114,17 +114,9 @@ ExecSetupPartitionTupleRouting(ModifyTableState *mtstate,
Relation rel)
* We need an additional tuple slot for storing transient
tuples that
* are converted to the root table descriptor.
*/
- proute->root_tuple_slot = MakeTupleTableSlot(NULL);
+ proute->root_tuple_slot =
MakeTupleTableSlot(RelationGetDescr(rel));
}
- /*
- * Initialize an empty slot that will be used to manipulate tuples of
any
- * given partition's rowtype. It is attached to the caller-specified
node
- * (such as ModifyTableState) and released when the node finishes
- * processing.
- */
- proute->partition_tuple_slot = MakeTupleTableSlot(NULL);
-
i = 0;
foreach(cell, leaf_parts)
{
@@ -709,6 +701,35 @@ ExecInitRoutingInfo(ModifyTableState *mtstate,
gettext_noop("could
not convert row type"));
/*
+ * If partition has different rowtype than root parent, initialize a
slot
+ * dedicated to storing this partition's tuples. The slot is used for
+ * various operations that are applied to tuple after routing, such as
+ * checking constraints.
+ */
+ if (proute->parent_child_tupconv_maps[partidx] != NULL)
+ {
+ Relation partrel = partRelInfo->ri_RelationDesc;
+
+ /*
+ * Initialize the array in proute where these slots are stored,
if not
+ * already done.
+ */
+ if (proute->partition_tuple_slots == NULL)
+ proute->partition_tuple_slots = (TupleTableSlot **)
+
palloc0(proute->num_partitions *
+
sizeof(TupleTableSlot *));
+
+ /*
+ * Initialize the slot itself setting its descriptor to this
+ * partition's TupleDesc; TupleDesc reference will be released
+ * at the end of the command.
+ */
+ proute->partition_tuple_slots[partidx] =
+
ExecInitExtraTupleSlot(estate,
+
RelationGetDescr(partrel));
+ }
+
+ /*
* If the partition is a foreign table, let the FDW init itself for
* routing tuples to the partition.
*/
@@ -801,8 +822,7 @@ ConvertPartitionTupleSlot(TupleConversionMap *map,
TupleTableSlot **p_my_slot,
bool shouldFree)
{
- if (!map)
- return tuple;
+ Assert(map != NULL && new_slot != NULL);
tuple = do_convert_tuple(tuple, map);
@@ -811,7 +831,6 @@ ConvertPartitionTupleSlot(TupleConversionMap *map,
*/
*p_my_slot = new_slot;
Assert(new_slot != NULL);
- ExecSetSlotDescriptor(new_slot, map->outdesc);
ExecStoreTuple(tuple, new_slot, InvalidBuffer, shouldFree);
return tuple;
@@ -885,8 +904,6 @@ ExecCleanupTupleRouting(ModifyTableState *mtstate,
/* Release the standalone partition tuple descriptors, if any */
if (proute->root_tuple_slot)
ExecDropSingleTupleTableSlot(proute->root_tuple_slot);
- if (proute->partition_tuple_slot)
- ExecDropSingleTupleTableSlot(proute->partition_tuple_slot);
}
/*
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index d8d89c7983..e4b2d374b4 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1161,11 +1161,12 @@ lreplace:;
map_index = resultRelInfo - mtstate->resultRelInfo;
Assert(map_index >= 0 && map_index <
mtstate->mt_nplans);
tupconv_map = tupconv_map_for_subplan(mtstate,
map_index);
- tuple = ConvertPartitionTupleSlot(tupconv_map,
-
tuple,
-
proute->root_tuple_slot,
-
&slot,
-
true);
+ if (tupconv_map != NULL)
+ tuple = ConvertPartitionTupleSlot(tupconv_map,
+
tuple,
+
proute->root_tuple_slot,
+
&slot,
+
true);
/*
* Prepare for tuple routing, making it look like we're
inserting
@@ -1703,6 +1704,7 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate,
int partidx;
ResultRelInfo *partrel;
HeapTuple tuple;
+ TupleConversionMap *map;
/*
* Determine the target partition. If ExecFindPartition does not find a
@@ -1790,11 +1792,16 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate,
/*
* Convert the tuple, if necessary.
*/
- ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[partidx],
- tuple,
-
proute->partition_tuple_slot,
- &slot,
- true);
+ map = proute->parent_child_tupconv_maps[partidx];
+ if (map != NULL)
+ {
+ TupleTableSlot *new_slot;
+
+ Assert(proute->partition_tuple_slots != NULL &&
+ proute->partition_tuple_slots[partidx] != NULL);
+ new_slot = proute->partition_tuple_slots[partidx];
+ ConvertPartitionTupleSlot(map, tuple, new_slot, &slot, true);
+ }
/* Initialize information needed to handle ON CONFLICT DO UPDATE. */
Assert(mtstate != NULL);
diff --git a/src/include/executor/execPartition.h
b/src/include/executor/execPartition.h
index f6cd842cc9..c2a2dc563e 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -86,10 +86,13 @@ typedef struct PartitionDispatchData *PartitionDispatch;
* element of this
array has the index into the
* corresponding
partition in partitions array.
* num_subplan_partition_offsets Length of 'subplan_partition_offsets' array
- * partition_tuple_slot TupleTableSlot to be used to
manipulate any
- * given leaf
partition's rowtype after that
- * partition is
chosen for insertion by
- * tuple-routing.
+ * partition_tuple_slots Array of TupleTableSlot objects; if
non-NULL,
+ * contains one
entry for every leaf partition,
+ * of which only
those of the leaf partitions
+ * whose attribute
numbers differ from the root
+ * parent have a
non-NULL value. NULL if all of
+ * the partitions
encountered by a given command
+ * happen to have
same rowtype as the root parent
* root_tuple_slot TupleTableSlot to be used to
transiently hold
* copy of a tuple
that's being moved across
* partitions in
the root partitioned table's
@@ -108,7 +111,7 @@ typedef struct PartitionTupleRouting
bool *child_parent_map_not_required;
int *subplan_partition_offsets;
int num_subplan_partition_offsets;
- TupleTableSlot *partition_tuple_slot;
+ TupleTableSlot **partition_tuple_slots;
TupleTableSlot *root_tuple_slot;
} PartitionTupleRouting;
--
2.11.0
