On Wed, Dec 13, 2017 at 5:18 AM, Amit Khandekar <amitdkhan...@gmail.com> wrote: > Amit Langote informed me off-list, - along with suggestions for > changes - that my patch needs a rebase. Attached is the rebased > version. I have also bumped the patch version number (now v29), > because this as additional changes, again, suggested by Amit L : > Because ExecSetupPartitionTupleRouting() has mtstate parameter now, > no need to pass update_rri and num_update_rri, since they can be > retrieved from mtstate. > > Also, the preparatory patch is also rebased.
Reviewing the preparatory patch: + PartitionTupleRouting *partition_tuple_routing; + /* Tuple-routing support info */ Something's wrong with the formatting here. - PartitionDispatch **pd, - ResultRelInfo ***partitions, - TupleConversionMap ***tup_conv_maps, - TupleTableSlot **partition_tuple_slot, - int *num_parted, int *num_partitions) + PartitionTupleRouting **partition_tuple_routing) Since we're consolidating all of ExecSetupPartitionTupleRouting's output parameters into a single structure, I think it might make more sense to have it just return that value. I think it's only done with output parameter today because there are so many different things being produced, and we can't return them all. + PartitionTupleRouting *ptr = mtstate->mt_partition_tuple_routing; This is just nitpicking, but I don't find "ptr" to be the greatest variable name; it looks too much like "pointer". Maybe we could use "routing" or "proute" or something. It seems to me that we could improve things here by adding a function ExecCleanupTupleRouting(PartitionTupleRouting *) which would do the various heap_close(), ExecDropSingleTupleTableSlot(), and ExecCloseIndices() operations which are currently performed in CopyFrom() and, by separate code, in ExecEndModifyTable(). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
