Re: [HACKERS] UPDATE of partition key - Mailing list pgsql-hackers
From | Amit Khandekar |
---|---|
Subject | Re: [HACKERS] UPDATE of partition key |
Date | |
Msg-id | CAJ3gD9c6Y=iLEB5wGmLHfxfwLGdHBvA9WhSNHbxjF2gG2R3kug@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] UPDATE of partition key (Amit Khandekar <amitdkhan.pg@gmail.com>) |
Responses |
Re: [HACKERS] UPDATE of partition key
(Amit Khandekar <amitdkhan.pg@gmail.com>)
|
List | pgsql-hackers |
On 29 November 2017 at 17:25, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: > Also, this > patch contains the incremental changes that were attached in the patch > encapsulate_partinfo.patch attached in [1]. In the next version, I > will extract them out again and keep them as a separate preparatory > patch. As mentioned above, attached is encapsulate_partinfo_preparatory.patch. This addresses David Rowley's request to move all the partition-related information into new structure PartitionTupleRouting, so that for ExecSetupPartitionTupleRouting(), we could pass pointer to this structure instead of the many parameters that we currently pass: [1] The main update-partition-key patch is to be applied over the above preparatory patch. Attached is its v27 version. This version addresses Thomas Munro's comments : On 14 November 2017 at 01:32, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Fri, Nov 10, 2017 at 4:42 PM, Amit Khandekar <amitdkhan.pg@gmail.com> wrote: >> Attached is v23 patch that has just the above changes (and also >> rebased on hash-partitioning changes, like update.sql). I am still >> doing some sanity testing on this, although regression passes. > > The test coverage[1] is 96.62%. Nice work. Here are the bits that > aren't covered: > > In partition.c's pull_child_partition_columns(), the following loop is > never run: > > + foreach(lc, partexprs) > + { > + Node *expr = (Node *) lfirst(lc); > + > + pull_varattnos(expr, 1, &child_keycols); > + } In update.sql, part_c_100_200 is now partitioned by range(abs(d)). So now the new function has_partition_atttrs() (in recent patch versions, this function has replaced pull_child_partition_columns) goes through the above code segment. This was indeed an important part left uncovered. Thanks. > > In nodeModifyTable.c, the following conditional branches are never run: > > if (mtstate->mt_oc_transition_capture != NULL) > + { > + Assert(mtstate->mt_is_tupconv_perpart == true); > mtstate->mt_oc_transition_capture->tcs_map = > - > mtstate->mt_transition_tupconv_maps[leaf_part_index]; > + > mtstate->mt_childparent_tupconv_maps[leaf_part_index]; > + } I think this code segment never hits even without the patch. For partitions, ON CONFLICT is not supported, and this code segment runs only for partitions. > > > if (node->mt_oc_transition_capture != NULL) > { > - > Assert(node->mt_transition_tupconv_maps != NULL); > > node->mt_oc_transition_capture->tcs_map = > - > node->mt_transition_tupconv_maps[node->mt_whichplan]; > + > tupconv_map_for_subplan(node, node->mt_whichplan); > } Here also, I verified that none of the regression tests hits this segment. The reason might be : this segment is run when an UPDATE starts with the next subplan, and mtstate->mt_oc_transition_capture is never allocated for UPDATEs. [1] : https://www.postgresql.org/message-id/CAJ3gD9f86H64e4OCjFFszWW7f4EeyriSaFL8SvJs2yOUbc8VEw%40mail.gmail.com -- Thanks, -Amit Khandekar EnterpriseDB Corporation The Postgres Database Company
Attachment
pgsql-hackers by date: