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
