Re: non-bulk inserts and tuple routing

Fujita-san,

Thanks for the review.

On 2018/02/16 18:12, Etsuro Fujita wrote:
> (2018/02/16 13:42), Amit Langote wrote:
>> Attached v9.  Thanks a for the review!
>
> Thanks for the updated patch!  In the patch you added the comments:
>
> +       wcoList = linitial(node->withCheckOptionLists);
> +
> +       /*
> +        * Convert Vars in it to contain this partition's attribute numbers.
> +        * Use the WITH CHECK OPTIONS list of the first resultRelInfo as a
> +        * reference to calculate attno's for the returning expression of
> +        * this partition.  In the INSERT case, that refers to the root
> +        * partitioned table, whereas in the UPDATE tuple routing case the
> +        * first partition in the mtstate->resultRelInfo array.  In any case,
> +        * both that relation and this partition should have the same
> columns,
> +        * so we should be able to map attributes successfully.
> +        */
> +       wcoList = map_partition_varattnos(wcoList, firstVarno,
> +                                         partrel, firstResultRel, NULL);
>
> This would be just nitpicking, but I think it would be better to arrange
> these comments, maybe by dividing these to something like this:
>
>        /*
>         * Use the WITH CHECK OPTIONS list of the first resultRelInfo as a
>         * reference to calculate attno's for the returning expression of
>         * this partition.  In the INSERT case, that refers to the root
>         * partitioned table, whereas in the UPDATE tuple routing case the
>         * first partition in the mtstate->resultRelInfo array.  In any case,
>         * both that relation and this partition should have the same columns,
>         * so we should be able to map attributes successfully.
>         */
>        wcoList = linitial(node->withCheckOptionLists);
>
>        /*
>         * Convert Vars in it to contain this partition's attribute numbers.
>         */
>        wcoList = map_partition_varattnos(wcoList, firstVarno,
>                                          partrel, firstResultRel, NULL);
>
> I'd say the same thing to the comments you added for RETURNING.

Good idea. Done.

> Another thing I noticed about comments in the patch is:
>
> +        * In the case of INSERT on partitioned tables, there is only one
> +        * plan.  Likewise, there is only one WCO list, not one per
> +        * partition.  For UPDATE, there would be as many WCO lists as
> +        * there are plans, but we use the first one as reference.  Note
> +        * that if there are SubPlans in there, they all end up attached
> +        * to the one parent Plan node.
>
> The patch calls ExecInitQual with mtstate->mt_plans[0] for initializing
> WCO constraints, which would change the place to attach SubPlans in WCO
> constraints from the parent PlanState to the subplan's PlanState, which
> would make the last comment obsolete.  Since this change would be more
> consistent with PG10, I don't think we need to update the comment as such;
> I would vote for just removing that comment from here.

I have thought about removing it too, so done.

Updated patch attached.

Thanks,
Amit