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
