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-07 10:36:37 |
Message-ID: | 5A7AD6B5.3020304@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
(2018/02/05 19:43), 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.
>> 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.
>> Here is the updated version that contains two patches as described above.
>
> Thanks for updating the patches! I'll post my next comments in a few days.
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
*));
* 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;
* I think it would be better to keep this comment here.
- /* Remember the subplan offset for this ResultRelInfo */
* Why is this removed from that initialization?
- proute->partitions[i] = leaf_part_rri;
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)
That's all I have for now.
Best regards,
Etsuro Fujita
From | Date | Subject | |
---|---|---|---|
Next Message | Etsuro Fujita | 2018-02-07 10:38:02 | Re: Incorrect grammar |
Previous Message | amul sul | 2018-02-07 10:12:27 | Re: In logical replication concurrent update of partition key creates a duplicate record on standby. |