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