Message-ID: <5A8578C1.20308@lab.ntt.co.jp>
Date: Thu, 15 Feb 2018 21:10:41 +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>
In-Reply-To: <4f144297-ba52-9f87-1f7a-d44fc7006d80@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/13 10:12), Amit Langote wrote:
> On 2018/02/09 21:20, Etsuro Fujita wrote:
>>>> * Please add a brief decsription about partition_oids to the comments for
>>>> this struct.
>>>>
>>>> @@ -91,6 +91,7 @@ typedef struct PartitionTupleRouting
>>>>    {
>>>>       PartitionDispatch *partition_dispatch_info;
>>>>       int         num_dispatch;
>>>> +   Oid        *partition_oids;
>>>
>>> Done.
>>
>> Thanks, but one thing I'm wondering is: do we really need this array?  I
>> think we could store into PartitionTupleRouting the list of oids returned
>> by RelationGetPartitionDispatchInfo in ExecSetupPartitionTupleRouting,
>> instead.  Sorry, I should have commented this in a previous email, but
>> what do you think about that?
>
> ExecInitPartitionInfo() will have to iterate the list to get the OID of
> the partition to be initialized.  Isn't it much cheaper with the array?

Good point!  So I agree with adding that array.

>> Here are other comments:
>>
>> o On changes to ExecSetupPartitionTupleRouting:
>>
>> * This is nitpicking, but it would be better to define partrel and
>> part_tupdesc within the if (update_rri_index<  num_update_rri&&
>> RelationGetRelid(update_rri[update_rri_index].ri_RelationDesc) ==
>> leaf_oid) block.
>>
>> -               ResultRelInfo *leaf_part_rri;
>> +               ResultRelInfo *leaf_part_rri = NULL;
>>                  Relation        partrel = NULL;
>>                  TupleDesc       part_tupdesc;
>>                  Oid                     leaf_oid = lfirst_oid(cell);
>
> Sure, done.
>
>> * Do we need this?  For a leaf partition that is already present in the
>> subplan resultrels, the partition's indices (if any) would have already
>> been opened.
>>
>> +                               /*
>> +                                * Open partition indices.  We wouldn't
>> need speculative
>> +                                * insertions though.
>> +                                */
>> +                               if
>> (leaf_part_rri->ri_RelationDesc->rd_rel->relhasindex&&
>> + leaf_part_rri->ri_IndexRelationDescs == NULL)
>> +                                       ExecOpenIndices(leaf_part_rri,
>> false);
>
> You're right.  Removed the call.

Thanks for the above changes!

> Updated patch is attached.

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

o On ExecInitPartitionInfo:

+   int         firstVarno;
+   Relation    firstResultRel;

My old compiler got "variable may be used uninitialized" warnings.

+   /*
+    * 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."

+       /*
+        * 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.

Other than that, the patch looks good to me.

Sorry for the delay.

Best regards,
Etsuro Fujita