Re: [HACKERS] UPDATE of partition key

Thanks for the updated patches, Amit.

Some review comments.

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.

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.

1. Patch to rename partition_tupconv_maps to parentchild_tupconv_maps.

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

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

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

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.

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.

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

Thanks,
Amit