(2018/02/09 14:32), Amit Langote wrote:
I had mistakenly tagged these patches v24, but they were actually supposed
to be v5. So the attached updated patch is tagged v6.
OK.
On 2018/02/07 19:36, Etsuro Fujita wrote:
(2018/02/05 14:34), Amit Langote wrote:
The code in tupconv_map_for_subplan() currently assumes that it can rely
on all leaf partitions having been initialized.
On reflection I noticed this analysis is not 100% correct; I think what
that function actually assumes is that all sublplans' partitions have
already been initialized, not all leaf partitions.
Yes, you're right.
Since we're breaking that
assumption with this proposal, that needed to be changed. So the patch
contained some refactoring to make it not rely on that assumption.
I don't think we really need this refactoring because since that as in
another patch you posted, we could initialize all subplans' partitions in
ExecSetupPartitionTupleRouting, I think tupconv_map_for_subplan could be
called without any changes to that function because of what I said above.
What my previous approach failed to consider is that in the update case,
we'd already have ResultRelInfo's for some of the leaf partitions
initialized, which could be saved into proute->partitions array right away.
Updated patch does things that way, so all the changes I had proposed to
tupconv_map_for_subplan are rendered unnecessary.
OK, thanks for the updated patch!
Here are comments for the other patch (patch
v24-0002-During-tuple-routing-initialize-per-partition-ob.patch):
o On changes to ExecSetupPartitionTupleRouting:
* The comment below wouldn't be correct; no ExecInitResultRelInfo in the
patch.
- proute->partitions = (ResultRelInfo **) palloc(proute->num_partitions *
- sizeof(ResultRelInfo *));
+ /*
+ * Actual ResultRelInfo's and TupleConversionMap's are allocated in
+ * ExecInitResultRelInfo().
+ */
+ proute->partitions = (ResultRelInfo **) palloc0(proute->num_partitions *
+ sizeof(ResultRelInfo
*));
I removed the comment altogether, as the comments elsewhere make the point
clear.
* The patch removes this from the initialization step for a subplan's
partition, but I think it would be better to keep this here because I
think it's a good thing to put the initialization stuff together into one
place.
- /*
- * This is required in order to we convert the partition's
- * tuple to be compatible with the root partitioned table's
- * tuple descriptor. When generating the per-subplan result
- * rels, this was not set.
- */
- leaf_part_rri->ri_PartitionRoot = rel;
It wasn't needed here with the previous approach, because we didn't touch
any ResultRelInfo's in ExecSetupPartitionTupleRouting, but I've added it
back in the updated patch.
* I think it would be better to keep this comment here.
- /* Remember the subplan offset for this ResultRelInfo */
Fixed.
* Why is this removed from that initialization?
- proute->partitions[i] = leaf_part_rri;
Because of the old approach. Now it's back in.
o On changes to ExecInitPartitionInfo:
* I don't understand the step starting from this, but I'm wondering if
that step can be removed by keeping the above setup of proute->partitions
for the subplan's partition in ExecSetupPartitionTupleRouting.
+ /*
+ * If we are doing tuple routing for update, try to reuse the
+ * per-subplan resultrel for this partition that ExecInitModifyTable()
+ * might already have created.
+ */
+ if (mtstate&& mtstate->operation == CMD_UPDATE)
Done, as mentioned above.
On 2018/02/08 19:16, Etsuro Fujita wrote:
Here are some minor comments:
o On changes to ExecInsert
* This might be just my taste, but I think it would be better to (1)
change ExecInitPartitionInfo so that it creates and returns a
newly-initialized ResultRelInfo for an initialized partition, and (2)
rewrite this bit:
+ /* Initialize partition info, if not done already. */
+ ExecInitPartitionInfo(mtstate, resultRelInfo, proute, estate,
+ leaf_part_index);
+
/*
* Save the old ResultRelInfo and switch to the one corresponding to
* the selected partition.
*/
saved_resultRelInfo = resultRelInfo;
resultRelInfo = proute->partitions[leaf_part_index];
+ Assert(resultRelInfo != NULL);
to something like this (I would say the same thing to the copy.c changes):
/*
* Save the old ResultRelInfo and switch to the one corresponding to
* the selected partition.
*/
saved_resultRelInfo = resultRelInfo;
resultRelInfo = proute->partitions[leaf_part_index];
if (resultRelInfo == NULL);
{
/* Initialize partition info. */
resultRelInfo = ExecInitPartitionInfo(mtstate,
saved_resultRelInfo,
proute,
estate,
leaf_part_index);
}
This would make ExecInitPartitionInfo more simple because it can assume
that the given partition has not been initialized yet.
Agree that it's much better to do it this way. Done.
Thanks for all those changes!
o On changes to execPartition.h
* Please add a brief decsription about partition_oids to the comments for
this struct.
@@ -91,6 +91,7 @@ typedef struct PartitionTupleRouting
{
PartitionDispatch *partition_dispatch_info;
int num_dispatch;
+ Oid *partition_oids;
Done.
Thanks, but one thing I'm wondering is: do we really need this array? I
think we could store into PartitionTupleRouting the list of oids
returned by RelationGetPartitionDispatchInfo in
ExecSetupPartitionTupleRouting, instead. Sorry, I should have commented
this in a previous email, but what do you think about that?
Here are other comments:
o On changes to ExecSetupPartitionTupleRouting:
* This is nitpicking, but it would be better to define partrel and
part_tupdesc within the if (update_rri_index < num_update_rri &&
RelationGetRelid(update_rri[update_rri_index].ri_RelationDesc) ==
leaf_oid) block.
- ResultRelInfo *leaf_part_rri;
+ ResultRelInfo *leaf_part_rri = NULL;
Relation partrel = NULL;
TupleDesc part_tupdesc;
Oid leaf_oid = lfirst_oid(cell);
* Do we need this? For a leaf partition that is already present in the
subplan resultrels, the partition's indices (if any) would have already
been opened.
+ /*
+ * Open partition indices. We wouldn't
need speculative
+ * insertions though.
+ */
+ if
(leaf_part_rri->ri_RelationDesc->rd_rel->relhasindex &&
+
leaf_part_rri->ri_IndexRelationDescs == NULL)
+ ExecOpenIndices(leaf_part_rri,
false);
I'll look at the patch a bit more early next week, but other than that,
the patch looks fairly in good shape to me.
Best regards,
Etsuro Fujita
