On 2018/07/28 10:54, David Rowley wrote:
> On 27 July 2018 at 19:11, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote:
>> I've attached a delta patch to make the above changes. I'm leaving the
>> hash table rename up to you though.
>
> Thanks for the delta patch. I took all of it, just rewrote a comment slightly.
>
> I also renamed the hash table to your suggestion and changed a few more
> things.
>
> Attached a delta based on v2 and the full v3 patch.
>
> This includes another small change to make
> PartitionDispatchData->indexes an array that's allocated in the same
> memory as the PartitionDispatchData. This will save a palloc() call
> and also should be a bit more cache friendly.
>
> This also required a rebase on master again due to 3e32109049.
Thanks for the updated patch.
I couldn't find much to complain about in the latest v3, except I noticed
a few instances of the word "setup" where I think what's really meant is
"set up".
+ * must be setup, but any sub-partitioned tables can be setup lazily as
+ * A ResultRelInfo has not been setup for this partition yet,
By the way, when going over the updated code, I noticed that the code
around child_parent_tupconv_maps could use some refactoring too.
Especially, I noticed that ExecSetupChildParentMapForLeaf() allocates
child-to-parent map array needed for transition tuple capture even if not
needed by any of the leaf partitions. I'm attaching here a patch that
applies on top of your v3 to show what I'm thinking we could do.
Thanks,
Amit
From 6ce1654aa929c7f8112c430914af7f464474ed31 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Mon, 30 Jul 2018 14:05:17 +0900
Subject: [PATCH 2/2] Some refactoring around child_parent_tupconv_maps
Just like parent_child_tupconv_maps, we should allocate it only if
needed. Also, if none of the partitions ended up needing a map, we
should not have allocated the child_parent_tupconv_maps array, only
the child_parent_map_not_required one. So, get rid of
ExecSetupChildParentMapForLeaf(), which currently does initial,
possibly useless, allocation of both of the above mentioned arrays.
Instead, have TupConvMapForLeaf() allocate the needed array, just
like ExecInitRoutingInfo does, when it needs to store a
parent-to-child map.
Finally, rename the function TupConvMapForLeaf to
LeafToParentTupConvMapForTC for clarity; TC stands for "Transition
Capture".
---
src/backend/commands/copy.c | 19 +-----
src/backend/executor/execPartition.c | 102 ++++++++++++++++-----------------
src/backend/executor/nodeModifyTable.c | 4 +-
src/include/executor/execPartition.h | 12 ++--
4 files changed, 59 insertions(+), 78 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 6fc1e2b41c..6d0e9229e0 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2503,22 +2503,9 @@ CopyFrom(CopyState cstate)
* CopyFrom tuple routing.
*/
if (cstate->rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- {
- PartitionTupleRouting *proute;
-
- proute = cstate->partition_tuple_routing =
+ cstate->partition_tuple_routing =
ExecSetupPartitionTupleRouting(NULL, cstate->rel);
- /*
- * If we are capturing transition tuples, they may need to be
- * converted from partition format back to partitioned table
format
- * (this is only ever necessary if a BEFORE trigger modifies the
- * tuple).
- */
- if (cstate->transition_capture != NULL)
- ExecSetupChildParentMapForLeaf(proute);
- }
-
/*
* It's more efficient to prepare a bunch of tuples for insertion, and
* insert them in one heap_multi_insert() call, than call heap_insert()
@@ -2666,8 +2653,8 @@ CopyFrom(CopyState cstate)
*/
cstate->transition_capture->tcs_original_insert_tuple = NULL;
cstate->transition_capture->tcs_map =
- TupConvMapForLeaf(proute,
saved_resultRelInfo,
-
leaf_part_index);
+
LeafToParentTupConvMapForTC(proute, saved_resultRelInfo,
+
leaf_part_index);
}
else
{
diff --git a/src/backend/executor/execPartition.c
b/src/backend/executor/execPartition.c
index 2a18a30b3e..d183e8b758 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -400,7 +400,7 @@ ExecExpandRoutingArrays(PartitionTupleRouting *proute)
sizeof(TupleConversionMap *) * (new_size -
old_size));
}
- if (proute->child_parent_map_not_required != NULL)
+ if (proute->child_parent_tupconv_maps != NULL)
{
proute->child_parent_tupconv_maps = (TupleConversionMap **)
repalloc(proute->child_parent_tupconv_maps,
@@ -940,73 +940,67 @@ ExecInitPartitionDispatchInfo(PartitionTupleRouting
*proute, Oid partoid,
}
/*
- * ExecSetupChildParentMapForLeaf -- Initialize the per-leaf-partition
- * child-to-root tuple conversion map array.
- *
- * This map is required for capturing transition tuples when the target table
- * is a partitioned table. For a tuple that is routed by an INSERT or UPDATE,
- * we need to convert it from the leaf partition to the target table
- * descriptor.
- */
-void
-ExecSetupChildParentMapForLeaf(PartitionTupleRouting *proute)
-{
- int size;
-
- Assert(proute != NULL);
-
- size = proute->partitions_allocsize;
-
- /*
- * These array elements get filled up with maps on an on-demand basis.
- * Initially just set all of them to NULL.
- */
- proute->child_parent_tupconv_maps =
- (TupleConversionMap **) palloc0(sizeof(TupleConversionMap *) *
size);
-
- /* Same is the case for this array. All the values are set to false */
- proute->child_parent_map_not_required = (bool *) palloc0(sizeof(bool) *
-
size);
-}
-
-/*
- * TupConvMapForLeaf -- Get the tuple conversion map for a given leaf partition
- * index.
+ * LeafToParentTupConvMapForTC -- Get the tuple conversion map to convert
+ * tuples of a leaf partition to the root parent's rowtype for storing in the
+ * transition capture tuplestore
*/
TupleConversionMap *
-TupConvMapForLeaf(PartitionTupleRouting *proute,
- ResultRelInfo *rootRelInfo, int leaf_index)
+LeafToParentTupConvMapForTC(PartitionTupleRouting *proute,
+ ResultRelInfo
*rootRelInfo,
+ int leaf_index)
{
- TupleConversionMap **map;
+ TupleConversionMap *map;
TupleDesc tupdesc;
- /* If nobody else set up the per-leaf maps array, do so ourselves. */
- if (proute->child_parent_tupconv_maps == NULL)
- ExecSetupChildParentMapForLeaf(proute);
+ Assert(leaf_index < proute->partitions_allocsize);
- /* If it's already known that we don't need a map, return NULL. */
- else if (proute->child_parent_map_not_required[leaf_index])
+ /* Did we already find out that we don't need a map for this partition?
*/
+ if (proute->child_parent_map_not_required &&
+ proute->child_parent_map_not_required[leaf_index])
return NULL;
- /* If we've already got a map, return it. */
- map = &proute->child_parent_tupconv_maps[leaf_index];
- if (*map != NULL)
- return *map;
-
- /* No map yet; try to create one. */
tupdesc =
RelationGetDescr(proute->partitions[leaf_index]->ri_RelationDesc);
- *map =
+ map =
convert_tuples_by_name(tupdesc,
RelationGetDescr(rootRelInfo->ri_RelationDesc),
gettext_noop("could
not convert row type"));
- /*
- * If it turns out no map is needed, remember that so we don't try
making
- * one again next time.
- */
- proute->child_parent_map_not_required[leaf_index] = (*map == NULL);
+ if (map)
+ {
+ /* If the per-leaf maps array has not been set up, do so
ourselves. */
+ if (proute->child_parent_tupconv_maps == NULL)
+ {
+ int size = proute->partitions_allocsize;
- return *map;
+ /*
+ * These array elements get filled up with maps on an
on-demand
+ * basis. Initially just set all of them to NULL.
+ */
+ proute->child_parent_tupconv_maps =
+ (TupleConversionMap **)
palloc0(sizeof(TupleConversionMap *) *
+
size);
+ }
+
+ proute->child_parent_tupconv_maps[leaf_index] = map;
+ }
+ else
+ {
+ if (proute->child_parent_map_not_required == NULL)
+ {
+ int size = proute->partitions_allocsize;
+
+ /*
+ * Values for other partitions will be filled whenever
they're
+ * selected by routing.
+ */
+ proute->child_parent_map_not_required =
+ (bool *) palloc0(sizeof(bool) * size);
+ }
+
+ proute->child_parent_map_not_required[leaf_index] = true;
+ }
+
+ return map;
}
/*
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 4f7cea7668..7d658ddad2 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1758,7 +1758,7 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate,
*/
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
mtstate->mt_transition_capture->tcs_map =
- TupConvMapForLeaf(proute, targetRelInfo,
partidx);
+ LeafToParentTupConvMapForTC(proute,
targetRelInfo, partidx);
}
else
{
@@ -1773,7 +1773,7 @@ ExecPrepareTupleRouting(ModifyTableState *mtstate,
if (mtstate->mt_oc_transition_capture != NULL)
{
mtstate->mt_oc_transition_capture->tcs_map =
- TupConvMapForLeaf(proute, targetRelInfo, partidx);
+ LeafToParentTupConvMapForTC(proute, targetRelInfo,
partidx);
}
/*
diff --git a/src/include/executor/execPartition.h
b/src/include/executor/execPartition.h
index d921ab6ca0..487a131343 100644
--- a/src/include/executor/execPartition.h
+++ b/src/include/executor/execPartition.h
@@ -125,9 +125,9 @@ typedef struct PartitionDispatchData *PartitionDispatch;
* the same size as
'partitions'.
*
* child_parent_map_not_required Stores if we don't need a conversion
- * map for a partition so
that TupConvMapForLeaf
- * can return without
having to re-check if it needs
- * to build a map.
+ * map for a partition so
that
+ *
LeafToParentTupConvMapForTC can return without
+ * having to re-check if
it needs to build a map.
*
* subplan_resultrel_hash Hash table to store subplan index by Oid.
*
@@ -244,9 +244,9 @@ extern void ExecInitRoutingInfo(ModifyTableState *mtstate,
PartitionTupleRouting *proute,
ResultRelInfo *partRelInfo,
int partidx);
-extern void ExecSetupChildParentMapForLeaf(PartitionTupleRouting *proute);
-extern TupleConversionMap *TupConvMapForLeaf(PartitionTupleRouting *proute,
- ResultRelInfo *rootRelInfo, int leaf_index);
+extern TupleConversionMap *LeafToParentTupConvMapForTC(PartitionTupleRouting
*proute,
+ ResultRelInfo
*rootRelInfo,
+ int leaf_index);
extern HeapTuple ConvertPartitionTupleSlot(TupleConversionMap *map,
HeapTuple tuple,
TupleTableSlot *new_slot,
--
2.11.0
