On Wed, Dec 13, 2017 at 5:18 AM, Amit Khandekar <amitdkhan.pg@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