Re: non-bulk inserts and tuple routing

Fujita-san,

Thanks a lot for the review.

I had mistakenly tagged these patches v24, but they were actually supposed
to be v5.  So the attached updated patch is tagged v6.

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.

> 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.

> 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.

Attached v6.

Thanks,
Amit