Re: non-bulk inserts and tuple routing

(2018/02/16 10:49), Amit Langote wrote:
> On 2018/02/15 21:10, Etsuro Fujita wrote:
>> here are some minor comments:
>>
>> o On changes to ExecCleanupTupleRouting:
>>
>> - ExecCloseIndices(resultRelInfo);
>> - heap_close(resultRelInfo->ri_RelationDesc, NoLock);
>> + if (resultRelInfo)
>> + {
>> + ExecCloseIndices(resultRelInfo);
>> + heap_close(resultRelInfo->ri_RelationDesc, NoLock);
>> + }
>>
>> You might check at the top of the loop whether resultRelInfo is NULL and
>> if so do continue. I think that would save cycles a bit.
>
> Good point, done.

Thanks.

>> o On ExecInitPartitionInfo:
>>
>> + int firstVarno;
>> + Relation firstResultRel;
>>
>> My old compiler got "variable may be used uninitialized" warnings.
>
> Fixed. Actually, I moved those declarations from out here into the blocks
> where they're actually needed.

OK, my compiler gets no warnings now.

>> + /*
>> + * Build the RETURNING projection if any for the partition. Note that
>> + * we didn't build the returningList for each partition within the
>> + * planner, but simple translation of the varattnos will suffice.
>> + * This only occurs for the INSERT case; in the UPDATE/DELETE cases,
>> + * ExecInitModifyTable() would've initialized this.
>> + */
>>
>> I think the last comment should be the same as for WCO lists: "This only
>> occurs for the INSERT case or in the case of UPDATE for which we didn't
>> find a result rel above to reuse."
>
> Fixed. The "above" is no longer needed, because there is no code left in
> ExecInitPartitionInfo() to find UPDATE result rels to reuse. That code is
> now in ExecSetupPartitionTupleRouting().

OK, that makes sense.

>> + /*
>> + * Initialize result tuple slot and assign its rowtype using the
>> first
>> + * RETURNING list. We assume the rest will look the same.
>> + */
>> + tupDesc = ExecTypeFromTL(returningList, false);
>> +
>> + /* Set up a slot for the output of the RETURNING projection(s) */
>> + ExecInitResultTupleSlot(estate,&mtstate->ps);
>> + ExecAssignResultType(&mtstate->ps, tupDesc);
>> + slot = mtstate->ps.ps_ResultTupleSlot;
>> +
>> + /* Need an econtext too */
>> + if (mtstate->ps.ps_ExprContext == NULL)
>> + ExecAssignExprContext(estate,&mtstate->ps);
>> + econtext = mtstate->ps.ps_ExprContext;
>>
>> Do we need this initialization? I think we would already have the slot
>> and econtext initialized when we get here.
>
> 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?

> Attached updated patch.

Thanks for updating the patch!

Best regards,
Etsuro Fujita