On Fri, Jan 5, 2018 at 3:25 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Fri, Jan 5, 2018 at 7:12 AM, Amit Khandekar <amitdkhan...@gmail.com> wrote: >> The above patch is to be applied over the last remaining preparatory >> patch, now named (and attached) : >> 0001-Refactor-CheckConstraint-related-code.patch > > Committed that one, too.
Some more comments on the main patch: I don't really like the fact that ExecCleanupTupleRouting() now takes a ModifyTableState as an argument, particularly because of the way that is using that argument. To figure out whether a ResultRelInfo was pre-existing or one it created, it checks whether the pointer address of the ResultRelInfo is >= mtstate->resultRelInfo and < mtstate->resultRelInfo + mtstate->mt_nplans. However, that means that ExecCleanupTupleRouting() ends up knowing about the memory allocation pattern used by ExecInitModifyTable(), which seems like a slightly dangerous amount of action at a distance. I think it would be better for the PartitionTupleRouting structure to explicitly indicate which ResultRelInfos should be closed, for example by storing a Bitmapset *input_partitions. (Here, by "input", I mean "provided from the mtstate rather than created by the PartitionTupleRouting structure; other naming suggestions welcome.) When ExecSetupPartitionTupleRouting latches onto a partition, it can do proute->input_partitions = bms_add_member(proute->input_partitons, i). In ExecCleanupTupleRouting, it can do if (bms_is_member(proute->input_partitions, i)) continue. We have a test, in the regression test suite for file_fdw, which generates the message "cannot route inserted tuples to a foreign table". I think we should have a similar test for the case where an UPDATE tries to move a tuple from a regular partition to a foreign table partition. I'm not sure if it should fail with the same error or a different one, but I think we should have a test that it fails cleanly and with a nice error message of some sort. The comment for get_partitioned_child_rels() claims that it sets is_partition_key_update, but it really sets *is_partition_key_update. And I think instead of "is a partition key" it should say "is used in the partition key either of the relation whose RTI is specified or of any child relation." I propose "used in" instead of "is" because there can be partition expressions, and the rest is to clarify that child partition keys matter. create_modifytable_path uses partColsUpdated rather than partKeyUpdated, which actually seems like better terminology. I propose partKeyUpdated -> partColsUpdated everywhere. Also, why use is_partition_key_update for basically the same thing in some other places? I propose changing that to partColsUpdated as well. The capitalization of the first comment hunk in execPartition.h is strange. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
