On Wed, Jul 5, 2017 at 4:50 PM, Dilip Kumar <dilipbal...@gmail.com> wrote: > On Mon, Jul 3, 2017 at 4:39 PM, amul sul <sula...@gmail.com> wrote: >> Thanks to catching this, fixed in the attached version. > > Few comments on the latest version. >
Thanks for your review, please find my comment inline:
> 0001 looks fine, for 0002 I have some comments.
>
> 1.
> + hbounds = (PartitionHashBound * *) palloc(nparts *
> + sizeof(PartitionHashBound *));
>
> /s/(PartitionHashBound * *)/(PartitionHashBound **)/g
>
Fixed in the attached version.
> 2.
> RelationBuildPartitionDesc
> {
> ....
>
>
> * catalog scan that retrieved them, whereas that in the latter is
> * defined by canonicalized representation of the list values or the
> * range bounds.
> */
> for (i = 0; i < nparts; i++)
> result->oids[mapping[i]] = oids[i];
>
> Should this comments mention about hash as well?
>
Instead, I have generalised this comment in the attached patch
> 3.
>
> if (b1->datums[b1->ndatums - 1][0] != b2->datums[b2->ndatums - 1][0])
> return false;
>
> if (b1->ndatums != b2->ndatums)
> return false;
>
> If ndatums itself is different then no need to access datum memory, so
> better to check ndatum first.
>
You are correct, we already doing this in the
partition_bounds_equal(). This is a redundant code, removed in the
attached version.
> 4.
> + * next larger modulus. For example, if you have a bunch
> + * of partitions that all have modulus 5, you can add a
> + * new new partition with modulus 10 or a new partition
>
> Typo, "new new partition" -> "new partition"
>
Fixed in the attached version.
Regards,
Amul
0001-Cleanup_v6.patch
Description: Binary data
0002-hash-partitioning_another_design-v15.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
