Message-ID: <5A7AD6B5.3020304@lab.ntt.co.jp>
Date: Wed, 07 Feb 2018 19:36:37 +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>
In-Reply-To: <5A783549.4020409@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/05 19:43), Etsuro Fujita wrote:
> (2018/02/05 14:34), Amit Langote wrote:
>> The code in tupconv_map_for_subplan() currently assumes that it can rely
>> on all leaf partitions having been initialized.

On reflection I noticed this analysis is not 100% correct; I think what 
that function actually assumes is that all sublplans' partitions have 
already been initialized, not all leaf partitions.

>> Since we're breaking that
>> assumption with this proposal, that needed to be changed. So the patch
>> contained some refactoring to make it not rely on that assumption.

I don't think we really need this refactoring because since that as in 
another patch you posted, we could initialize all subplans' partitions 
in ExecSetupPartitionTupleRouting, I think tupconv_map_for_subplan could 
be called without any changes to that function because of what I said above.

>> Here is the updated version that contains two patches as described above.
>
> Thanks for updating the patches!  I'll post my next comments in a few days.

Here are comments for the other patch (patch 
v24-0002-During-tuple-routing-initialize-per-partition-ob.patch):

o On changes to ExecSetupPartitionTupleRouting:

* The comment below wouldn't be correct; no ExecInitResultRelInfo in the 
patch.

-   proute->partitions = (ResultRelInfo **) palloc(proute->num_partitions *
-                                                  sizeof(ResultRelInfo *));
+   /*
+    * Actual ResultRelInfo's and TupleConversionMap's are allocated in
+    * ExecInitResultRelInfo().
+    */
+   proute->partitions = (ResultRelInfo **) palloc0(proute->num_partitions *
+                                                   sizeof(ResultRelInfo 
*));

* The patch removes this from the initialization step for a subplan's 
partition, but I think it would be better to keep this here because I 
think it's a good thing to put the initialization stuff together into 
one place.

-               /*
-                * This is required in order to we convert the partition's
-                * tuple to be compatible with the root partitioned table's
-                * tuple descriptor.  When generating the per-subplan result
-                * rels, this was not set.
-                */
-               leaf_part_rri->ri_PartitionRoot = rel;

* I think it would be better to keep this comment here.

-               /* Remember the subplan offset for this ResultRelInfo */

* Why is this removed from that initialization?

-       proute->partitions[i] = leaf_part_rri;

o On changes to ExecInitPartitionInfo:

* I don't understand the step starting from this, but I'm wondering if 
that step can be removed by keeping the above setup of 
proute->partitions for the subplan's partition in 
ExecSetupPartitionTupleRouting.

+   /*
+    * If we are doing tuple routing for update, try to reuse the
+    * per-subplan resultrel for this partition that ExecInitModifyTable()
+    * might already have created.
+    */
+   if (mtstate && mtstate->operation == CMD_UPDATE)

That's all I have for now.

Best regards,
Etsuro Fujita