Re: non-bulk inserts and tuple routing
От | Amit Langote |
---|---|
Тема | Re: non-bulk inserts and tuple routing |
Дата | |
Msg-id | b34e69b2-5d67-29f1-7a35-6b6299243ec0@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: non-bulk inserts and tuple routing (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
Ответы |
Re: non-bulk inserts and tuple routing
(Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>)
|
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: