Re: non-bulk inserts and tuple routing

(2018/02/16 18:23), Amit Langote wrote:
> On 2018/02/16 18:12, Etsuro Fujita wrote:
>> 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.

Thanks.  I fixed a typo (s/Converti/Convert/) and adjusted these 
comments a bit further to match the preceding code/comments.  Attached 
is the updated version.

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

OK.

> Updated patch attached.

Thanks, I think the patch is in good shape, so I'll mark this as ready 
for committer.

Best regards,
Etsuro Fujita