Re: non-bulk inserts and tuple routing

(2018/01/30 18:52), Etsuro Fujita wrote:
> (2018/01/30 18:39), Amit Langote wrote:
>> Will wait for your comments before sending a new version then.
>
> Ok, I'll post my comments as soon as possible.

* ExecInitPartitionResultRelInfo is called from ExecFindPartition, but 
we could call that another way; in ExecInsert/CopyFrom we call that 
after ExecFindPartition if the partition chosen by ExecFindPartition has 
not been initialized yet.  Maybe either would be OK, but I like that 
because I think that would not only better divide that labor but better 
fit into the existing code in ExecInsert/CopyFrom IMO.

* In ExecInitPartitionResultRelInfo:
+       /*
+        * Note that the entries in this list appear in no predetermined
+        * order as result of initializing partition result rels as and when
+        * they're needed.
+        */
+       estate->es_leaf_result_relations =
+ 
lappend(estate->es_leaf_result_relations,
+                                           leaf_part_rri);

Is it OK to put this within the "if (leaf_part_rri == NULL)" block?

* In the same function:
+   /*
+    * Verify result relation is a valid target for INSERT.
+    */
+   CheckValidResultRel(leaf_part_rri, CMD_INSERT);

I think it would be better to leave the previous comments as-is here:

         /*
          * Verify result relation is a valid target for an INSERT.  An 
UPDATE
          * of a partition-key becomes a DELETE+INSERT operation, so 
this check
          * is still required when the operation is CMD_UPDATE.
          */

* ExecInitPartitionResultRelInfo does the work other than the 
initialization of ResultRelInfo for the chosen partition (eg, create a 
tuple conversion map to convert a tuple routed to the partition from the 
parent's type to the partition's).  So I'd propose to rename that 
function to eg, ExecInitPartition.

* CopyFrom is modified so that it calls ExecSetupPartitionTupleRouting 
and ExecFindPartition with a mostly-dummy ModifyTableState node.  I'm 
not sure that is a good idea.  My concern about that is that might be 
something like a headache in future development.

* The patch 0001 and 0002 are pretty small but can't be reviewed without 
the patch 0003.  The total size of the three patches aren't that large, 
so I think it would be better to put those patches together into a 
single patch.

That's all for now.  I'll continue to review the patches!

Best regards,
Etsuro Fujita