Re: [HACKERS] UPDATE of partition key

On 14 December 2017 at 08:11, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Forgot to remove the description of update_rri and num_update_rri in the
> header comment of ExecSetupPartitionTupleRouting().
>
> -
> +extern void pull_child_partition_columns(Relation rel,
> + Relation parent,
> + Bitmapset **partcols);
>
> It seems you forgot to remove this declaration in partition.h, because I
> don't find it defined or used anywhere.

Done both of the above. Attached v30 patch has the above changes.

>
> I think some of the changes that are currently part of the main patch are
> better taken out into their own patches, because having those diffs appear
> in the main patch is kind of distracting. Just like you now have a patch
> that introduces a PartitionTupleRouting structure. I know that leads to
> too many patches, but it helps to easily tell less substantial changes
> from the substantial ones.

Done. Created patches as shown below :

>
> 1. Patch to rename partition_tupconv_maps to parentchild_tupconv_maps.

As per Robert's suggestion, reverted back the renaming of this field.

>
> 2. Patch that introduces has_partition_attrs() in place of
> is_partition_attr()

0002-Changed-is_partition_attr-to-has_partition_attrs.patch

>
> 3. Patch to change the names of map_partition_varattnos() arguments

0003-Renaming-parameters-of-map_partition_var_attnos.patch

>
> 4. Patch that does the refactoring involving ExecConstrains(),
> ExecPartitionCheck(), and the introduction of
> ExecPartitionCheckEmitError()

0004-Refactor-CheckConstraint-related-code.patch

The preparatory patches are to be applied in order of the patch
numbers, followed by the main patch update-partition-key_v30.patch

>
>
> Regarding ExecSetupChildParentMap(), it seems to me that it could simply
> be declared as
>
> static void ExecSetupChildParentMap(ModifyTableState *mtstate);
>
> Looking at the places from where it's called, it seems that you're just
> extracting information from mtstate and passing the same for the rest of
> its arguments.
>

Agreed. But the last parameter per_leaf might be necessary. I will
defer this until I address Robert's concern about the complexity of
the related code.

> mt_is_tupconv_perpart seems like it's unnecessary. Its function could be
> fulfilled by inspecting the state of some other fields of
> ModifyTableState. For example, in the case of an update (operation ==
> CMD_UPDATE), if mt_partition_tuple_routing is non-NULL, then we can always
> assume that mt_childparent_tupconv_maps has entries for all partitions.
> If it's NULL, then there would be only entries for partitions that have
> sub-plans.

I think we better have this field separately for code-clarity, and to
avoid repeated execution of multiple conditions, and in order to have
some signficant Asserts() that use this field.

>
> tupconv_map_for_subplan() looks like it could be done as a macro.

Or may be inline function. I will again defer this for similar reason
as the above deferred item about ExecSetupChildParentMap parameters.

--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company