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, 09 Feb 2018 21:20:03 +0900
Message-ID: <5A7D91F3.9050309@lab.ntt.co.jp> (raw)
In-Reply-To: <481cc9d5-2dd8-b3f4-23b0-ecbccd8a85e0@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>
<5A7443DF.1010408@lab.ntt.co.jp>
<14870d0e-d9d4-2ab9-ddfe-182b3c8abfca@lab.ntt.co.jp>
<5A783549.4020409@lab.ntt.co.jp>
<5A7AD6B5.3020304@lab.ntt.co.jp>
<5A7C2364.9040700@lab.ntt.co.jp>
<481cc9d5-2dd8-b3f4-23b0-ecbccd8a85e0@lab.ntt.co.jp>
(2018/02/09 14:32), Amit Langote wrote:
> I had mistakenly tagged these patches v24, but they were actually supposed
> to be v5. So the attached updated patch is tagged v6.
OK.
> On 2018/02/07 19:36, 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.
>
> Yes, you're right.
>
>>>> 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.
>
> What my previous approach failed to consider is that in the update case,
> we'd already have ResultRelInfo's for some of the leaf partitions
> initialized, which could be saved into proute->partitions array right away.
>
> Updated patch does things that way, so all the changes I had proposed to
> tupconv_map_for_subplan are rendered unnecessary.
OK, thanks for the updated patch!
>> 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
> *));
>
> I removed the comment altogether, as the comments elsewhere make the point
> clear.
>
>> * 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;
>
> It wasn't needed here with the previous approach, because we didn't touch
> any ResultRelInfo's in ExecSetupPartitionTupleRouting, but I've added it
> back in the updated patch.
>
>> * I think it would be better to keep this comment here.
>>
>> - /* Remember the subplan offset for this ResultRelInfo */
>
> Fixed.
>
>> * Why is this removed from that initialization?
>>
>> - proute->partitions[i] = leaf_part_rri;
>
> Because of the old approach. Now it's back in.
>
>> 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)
>
> Done, as mentioned above.
>
> On 2018/02/08 19:16, Etsuro Fujita wrote:
>> Here are some minor comments:
>>
>> o On changes to ExecInsert
>>
>> * This might be just my taste, but I think it would be better to (1)
>> change ExecInitPartitionInfo so that it creates and returns a
>> newly-initialized ResultRelInfo for an initialized partition, and (2)
>> rewrite this bit:
>>
>> + /* Initialize partition info, if not done already. */
>> + ExecInitPartitionInfo(mtstate, resultRelInfo, proute, estate,
>> + leaf_part_index);
>> +
>> /*
>> * Save the old ResultRelInfo and switch to the one corresponding to
>> * the selected partition.
>> */
>> saved_resultRelInfo = resultRelInfo;
>> resultRelInfo = proute->partitions[leaf_part_index];
>> + Assert(resultRelInfo != NULL);
>>
>> to something like this (I would say the same thing to the copy.c changes):
>>
>> /*
>> * Save the old ResultRelInfo and switch to the one corresponding to
>> * the selected partition.
>> */
>> saved_resultRelInfo = resultRelInfo;
>> resultRelInfo = proute->partitions[leaf_part_index];
>> if (resultRelInfo == NULL);
>> {
>> /* Initialize partition info. */
>> resultRelInfo = ExecInitPartitionInfo(mtstate,
>> saved_resultRelInfo,
>> proute,
>> estate,
>> leaf_part_index);
>> }
>>
>> This would make ExecInitPartitionInfo more simple because it can assume
>> that the given partition has not been initialized yet.
>
> Agree that it's much better to do it this way. Done.
Thanks for all those changes!
>> o On changes to execPartition.h
>>
>> * Please add a brief decsription about partition_oids to the comments for
>> this struct.
>>
>> @@ -91,6 +91,7 @@ typedef struct PartitionTupleRouting
>> {
>> PartitionDispatch *partition_dispatch_info;
>> int num_dispatch;
>> + Oid *partition_oids;
>
> Done.
Thanks, but one thing I'm wondering is: do we really need this array? I
think we could store into PartitionTupleRouting the list of oids
returned by RelationGetPartitionDispatchInfo in
ExecSetupPartitionTupleRouting, instead. Sorry, I should have commented
this in a previous email, but what do you think about that?
Here are other comments:
o On changes to ExecSetupPartitionTupleRouting:
* This is nitpicking, but it would be better to define partrel and
part_tupdesc within the if (update_rri_index < num_update_rri &&
RelationGetRelid(update_rri[update_rri_index].ri_RelationDesc) ==
leaf_oid) block.
- ResultRelInfo *leaf_part_rri;
+ ResultRelInfo *leaf_part_rri = NULL;
Relation partrel = NULL;
TupleDesc part_tupdesc;
Oid leaf_oid = lfirst_oid(cell);
* Do we need this? For a leaf partition that is already present in the
subplan resultrels, the partition's indices (if any) would have already
been opened.
+ /*
+ * Open partition indices. We wouldn't
need speculative
+ * insertions though.
+ */
+ if
(leaf_part_rri->ri_RelationDesc->rd_rel->relhasindex &&
+
leaf_part_rri->ri_IndexRelationDescs == NULL)
+ ExecOpenIndices(leaf_part_rri,
false);
I'll look at the patch a bit more early next week, but other than that,
the patch looks fairly in good shape to me.
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: <5A7D91F3.9050309@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