Re: non-bulk inserts and tuple routing - Mailing list pgsql-hackers
From | Etsuro Fujita |
---|---|
Subject | Re: non-bulk inserts and tuple routing |
Date | |
Msg-id | 5A86B77E.1010306@lab.ntt.co.jp Whole thread Raw |
In response to | Re: non-bulk inserts and tuple routing (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Responses |
Re: non-bulk inserts and tuple routing
|
List | pgsql-hackers |
(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
Attachment
pgsql-hackers by date: