On Wed, May 10, 2017 at 11:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, May 3, 2017 at 9:09 AM, amul sul <sulamul@gmail.com> wrote:
>> Fixed in the attached version.
>
> +[ PARTITION BY { HASH | RANGE | LIST } ( { <replaceable
> class="parameter">column_name</replaceable> | ( <replaceable
> class="parameter">expression</replaceable> ) } [ COLLATE <replaceable
>
> In the department of severe nitpicking, I would have expected this to
> either use alphabetical order (HASH | LIST | RANGE) or to add the new
> method at the end on the theory that we probably did the important
> ones first (RANGE | LIST | HASH).
Importance is subjective, so may be we should arrange them in
alphabetical order, to keep the list in some order and be consistent
everywhere in the code and documentation.
> More broadly, I wonder why we're
> cramming this into the datums arrays instead of just adding another
> field to PartitionBoundInfoData that is only used by hash
> partitioning.
It would be good if we store datums corresponding to partition bounds
in the same place. So that we don't have to handle hash partition
specially in all the places where we handle partition bound datums. We
already do that for list and range partitions. May be we want to
continue doing so for hash as well. In my comments to Amul's latest
patch, I have described a possibility that partition_bounds_equal()
need not compare all entries in the datums array. It can just compare
greated modulus and the indexes array from given two partition bounds
to check whether they are equal. If that works well, we will probably
address your complaint about DatumIsEqual() in a different manner.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company