(2018/02/05 19:43), 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.
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.
Here is the updated version that contains two patches as described above.
Thanks for updating the patches! I'll post my next comments in a few days.
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
*));
* 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;
* I think it would be better to keep this comment here.
- /* Remember the subplan offset for this ResultRelInfo */
* Why is this removed from that initialization?
- proute->partitions[i] = leaf_part_rri;
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)
That's all I have for now.
Best regards,
Etsuro Fujita
