Re: [HACKERS] UPDATE of partition key

On 29 November 2017 at 17:25, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)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(dot)munro(at)enterprisedb(dot)com> wrote:
> On Fri, Nov 10, 2017 at 4:42 PM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)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