On 15 December 2017 at 18:28, Robert Haas <robertmhaas@gmail.com> wrote:
> Reviewing the preparatory patch:
>
> + PartitionTupleRouting *partition_tuple_routing;
> + /* Tuple-routing support info */
>
> Something's wrong with the formatting here.
Moved the comment above the declaration.
>
> - 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.
You mean ExecSetupPartitionTupleRouting() will return the structure
(not pointer to structure), and the caller will get the copy of the
structure like this ? :
mtstate->mt_partition_tuple_routing =
ExecSetupPartitionTupleRouting(mtstate, rel, node->nominalRelation, estate);
I am ok with that, but just wanted to confirm if that is what you are
saying. I don't recall seeing a structure return value in PG code, so
not sure if it is conventional in PG to do that. Hence, I am somewhat
inclined to keep it as output param. It also avoids a structure copy.
Another way is for ExecSetupPartitionTupleRouting() to palloc this
structure, and return its pointer, but then caller would have to
anyway do a structure copy, so that's not convenient, and I don't
think you are suggesting this way either.
>
> + 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.
Done. Renamed it to "proute".
>
> 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().
>
Done. Changes are kept in a new preparatory patch
0005-Organize-cleanup-done-for-partition-tuple-routing.patch
Yet to address your other review comments.
Attached is patch v31. (Preparatory patches to be applied in order of
patch numbers, followed by the main patch)
Thanks
-Amit
