Message-ID: <5A8652E3.9000009@lab.ntt.co.jp>
Date: Fri, 16 Feb 2018 12:41:23 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: non-bulk inserts and tuple routing
References: <8975331d-d961-cbdd-f862-fdd3d97dc2d0@lab.ntt.co.jp>
 <f3a0d60e-1f60-f2ea-dd6e-89c397e4ecf4@lab.ntt.co.jp>
 <CA+TgmoZM7S8cakW4j4b5xOz=p3ZOaMJTK-zd8ca8Gh9gY-17Tw@mail.gmail.com>
 <56c9f77b-830c-4652-233e-a1c5fa5a9938@lab.ntt.co.jp>
 <8f53a342-e0df-4fc9-7422-c3c3c601117c@lab.ntt.co.jp>
 <5A69A3B7.10403@lab.ntt.co.jp>
 <37f7cc10-def8-b2d0-615d-dfe035e7b1ee@lab.ntt.co.jp>
 <5A70396A.8090400@lab.ntt.co.jp>
 <8399fd28-07c2-3cbc-f807-f89cdf2f98f5@lab.ntt.co.jp>
 <5A704055.40207@lab.ntt.co.jp> <5A7443DF.1010408@lab.ntt.co.jp>
 <14870d0e-d9d4-2ab9-ddfe-182b3c8abfca@lab.ntt.co.jp>
 <5A783549.4020409@lab.ntt.co.jp> <5A7AD6B5.3020304@lab.ntt.co.jp>
 <5A7C2364.9040700@lab.ntt.co.jp>
 <481cc9d5-2dd8-b3f4-23b0-ecbccd8a85e0@lab.ntt.co.jp>
 <5A7D91F3.9050309@lab.ntt.co.jp>
 <4f144297-ba52-9f87-1f7a-d44fc7006d80@lab.ntt.co.jp>
 <5A8578C1.20308@lab.ntt.co.jp>
 <c0c02a3f-5eba-77c5-5476-95e1f003623e@lab.ntt.co.jp>
In-Reply-To: <c0c02a3f-5eba-77c5-5476-95e1f003623e@lab.ntt.co.jp>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: pgsql-hackers@lists.postgresql.org
Precedence: bulk

(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