Re: [POC] hash partitioning

On Wed, May 3, 2017 at 6:39 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
> On Thu, Apr 27, 2017 at 1:42 AM, Robert Haas <robertmhaas(at)gmail(dot)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
* partitioned tables */

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; /* Index of 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