From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: non-bulk inserts and tuple routing
Date: Fri, 02 Feb 2018 19:56:31 +0900
Message-ID: <5A7443DF.1010408@lab.ntt.co.jp> (raw)
In-Reply-To: <5A704055.40207@lab.ntt.co.jp>
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>
(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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: fujita.etsuro@lab.ntt.co.jp, Langote_Amit_f8@lab.ntt.co.jp, pgsql-hackers@lists.postgresql.org
Subject: Re: non-bulk inserts and tuple routing
In-Reply-To: <5A7443DF.1010408@lab.ntt.co.jp>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox