Re: non-bulk inserts and tuple routing

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: non-bulk inserts and tuple routing
Date: 2018-02-02 10:56:31
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

(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 =
+ 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
* 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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andrey Borodin 2018-02-02 12:22:07 Re: [HACKERS] Can ICU be used for a database's default sort order?
Previous Message Etsuro Fujita 2018-02-02 10:43:40 Re: [HACKERS] Add support for tuple routing to foreign partitions