Re: non-bulk inserts and tuple routing

(2018/02/16 13:42), Amit Langote wrote:
> On 2018/02/16 12:41, Etsuro Fujita wrote:
>> (2018/02/16 10:49), Amit Langote wrote:
>>> I think you're right. If node->returningLists is non-NULL at all,
>>> ExecInitModifyTable() would've initialized the needed slot and expression
>>> context. I added Assert()s to that affect.
>>
>> OK, but one thing I'd like to ask is:
>>
>> + /*
>> + * Use the slot that would have been set up in ExecInitModifyTable()
>> + * for the output of the RETURNING projection(s). Just make sure to
>> + * assign its rowtype using the RETURNING list.
>> + */
>> + Assert(mtstate->ps.ps_ResultTupleSlot != NULL);
>> + tupDesc = ExecTypeFromTL(returningList, false);
>> + ExecAssignResultType(&mtstate->ps, tupDesc);
>> + slot = mtstate->ps.ps_ResultTupleSlot;
>>
>> Do we need that assignment here?
>
> I guess mean the assignment of rowtype, that is, the
> ExecAssignResultType() line.

That's right.

> On looking at this some more, it looks like
> we don't need to ExecAssignResultType here, as you seem to be suspecting,
> because we want the RETURNING projection output to use the rowtype of the
> first of returningLists and that's what mtstate->ps.ps_ResultTupleSlot has
> been set to use in the first place.

Year, I think so, too.

> So, removed the ExecAssignResultType().

OK, thinks.

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

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.

Best regards,
Etsuro Fujita