Re: non-bulk inserts and tuple routing

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Etsuro Fujita <fujita(dot)etsuro(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-16 09:23:55
Message-ID: b34e69b2-5d67-29f1-7a35-6b6299243ec0@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Fujita-san,

Thanks for the review.

On 2018/02/16 18:12, Etsuro Fujita wrote:
> (2018/02/16 13:42), Amit Langote wrote:
>> Attached v9.  Thanks a for the review!
>
> Thanks for the updated patch!  In the patch you added the comments:
>
> +       wcoList = linitial(node->withCheckOptionLists);
> +
> +       /*
> +        * Convert Vars in it to contain this partition's attribute numbers.
> +        * Use the WITH CHECK OPTIONS list of the first resultRelInfo as a
> +        * reference to calculate attno's for the returning expression of
> +        * this partition.  In the INSERT case, that refers to the root
> +        * partitioned table, whereas in the UPDATE tuple routing case the
> +        * first partition in the mtstate->resultRelInfo array.  In any case,
> +        * both that relation and this partition should have the same
> columns,
> +        * so we should be able to map attributes successfully.
> +        */
> +       wcoList = map_partition_varattnos(wcoList, firstVarno,
> +                                         partrel, firstResultRel, NULL);
>
> This would be just nitpicking, but I think it would be better to arrange
> these comments, maybe by dividing these to something like this:
>
>        /*
>         * Use the WITH CHECK OPTIONS list of the first resultRelInfo as a
>         * reference to calculate attno's for the returning expression of
>         * this partition.  In the INSERT case, that refers to the root
>         * partitioned table, whereas in the UPDATE tuple routing case the
>         * first partition in the mtstate->resultRelInfo array.  In any case,
>         * both that relation and this partition should have the same columns,
>         * so we should be able to map attributes successfully.
>         */
>        wcoList = linitial(node->withCheckOptionLists);
>
>        /*
>         * Convert Vars in it to contain this partition's attribute numbers.
>         */
>        wcoList = map_partition_varattnos(wcoList, firstVarno,
>                                          partrel, firstResultRel, NULL);
>
> I'd say the same thing to the comments you added for RETURNING.

Good idea. Done.

> Another thing I noticed about comments in the patch is:
>
> +        * In the case of INSERT on partitioned tables, there is only one
> +        * plan.  Likewise, there is only one WCO list, not one per
> +        * partition.  For UPDATE, there would be as many WCO lists as
> +        * there are plans, but we use the first one as reference.  Note
> +        * that if there are SubPlans in there, they all end up attached
> +        * to the one parent Plan node.
>
> The patch calls ExecInitQual with mtstate->mt_plans[0] for initializing
> WCO constraints, which would change the place to attach SubPlans in WCO
> constraints from the parent PlanState to the subplan's PlanState, which
> would make the last comment obsolete.  Since this change would be more
> consistent with PG10, I don't think we need to update the comment as such;
> I would vote for just removing that comment from here.

I have thought about removing it too, so done.

Updated patch attached.

Thanks,
Amit

Attachment Content-Type Size
v10-0001-During-tuple-routing-initialize-per-partition-ob.patch text/plain 22.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message amul sul 2018-02-16 09:36:26 Re: Server crash in pg_replication_slot_advance function
Previous Message Etsuro Fujita 2018-02-16 09:12:51 Re: non-bulk inserts and tuple routing