Re: non-bulk inserts and tuple routing

Fujita-san,

Thank you for the review.

On 2018/02/02 19:56, Etsuro Fujita wrote:
> (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.

I see no problem with that, so done that way.

> * 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?

Good catch. I moved it outside the block. I was under the impression
that leaf result relations that were reused from the
mtstate->resultRelInfo arrary would have already been added to the list,
but it seems they are not.

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

Oops, my bad. Didn't notice that I had ended up removing the part about
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.

I went with ExevInitPartitionInfo.

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

OK, I removed those changes.

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

As I said above, I got rid of 0001. Then, I merged the
ExecFindPartition() refactoring patch 0002 into 0003.

The code in tupconv_map_for_subplan() currently assumes that it can rely
on all leaf partitions having been initialized. 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
carved that out into a separate patch which can be applied and tested
before the main patch.

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

Here is the updated version that contains two patches as described above.

Thanks,
Amit