Re: [HACKERS] [POC] hash partitioning

On Wed, May 3, 2017 at 6:39 PM, amul sul <sulamul@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 1:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
>>I spent some time today looking at these patches.  It seems like there
>>is some more work still needed here to produce something committable
>>regardless of which way we go, but I am inclined to think that Amul's
>>patch is a better basis for work going forward than Nagata-san's
>>patch. Here are some general comments on the two patches:
>
> Thanks for your time.
>
> [...]
>
>> - Neither patch contains any documentation updates, which is bad.
>
> Fixed in the attached version.

I have done an intial review of the patch and I have some comments.  I
will continue the review
and testing and report the results soon

-----
Patch need to be rebased

----

if (key->strategy == PARTITION_STRATEGY_RANGE)
{
/* Disallow nulls in the range partition key of the tuple */
for (i = 0; i < key->partnatts; i++)
if (isnull[i])
ereport(ERROR,
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
errmsg("range partition key of row contains null")));
}

We need to add PARTITION_STRATEGY_HASH as well, we don't support NULL
for hash also, right?
----

RangeDatumContent **content;/* what's contained in each range bound datum? * (see the above enum); NULL for list *
partitionedtables */
 

This will be NULL for hash as well we need to change the comments.
-----
 bool has_null; /* Is there a null-accepting partition? false * for range partitioned tables */ int null_index; /*
Indexof the null-accepting partition; -1
 

Comments needs to be changed for these two members as well
----

+/* One bound of a hash partition */
+typedef struct PartitionHashBound
+{
+ int modulus;
+ int remainder;
+ int index;
+} PartitionHashBound;

It will good to add some comments to explain the structure members


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com