On Thu, May 11, 2017 at 9:32 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> 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?
> ----
We do.
>
> 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.
> -----
Fixed in previously posted patch(v3).
>
> 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
> ----
Fixed in previously posted patch(v3).
>
> +/* 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
>
I think we don't really need that, variable names are ample to explain
its purpose.
Regards,
Amul
