From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> |
Cc: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> |
Subject: | Re: [HACKERS] UPDATE of partition key |
Date: | 2017-12-15 12:58:07 |
Message-ID: | CA+TgmoZYQ5wjv4OX=hRB2WVjTiGskMMeedhhMeeVdwZ+xkMXUw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Dec 13, 2017 at 5:18 AM, Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2017-12-15 13:30:14 | Re: GSoC 2018 |
Previous Message | Aleksander Alekseev | 2017-12-15 12:57:50 | Re: GSoC 2018 |