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