Hi Dean,
Thanks a lot for the review.
On 2017/07/03 1:59, Dean Rasheed wrote:
> On 30 June 2017 at 09:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> When testing the patch, I realized that the current code in
>> check_new_partition_bound() that checks for range partition overlap had a
>> latent bug that resulted in false positives for the new cases that the new
>> less restrictive syntax allowed. I spent some time simplifying that code
>> while also fixing the aforementioned bug. It's implemented in the
>> attached patch 0001.
>>
>
> I haven't had time to look at 0002 yet, but looking at 0001, I'm not
> convinced that this really represents much of a simplification, but I
> do prefer the way it now consistently reports the first overlapping
> partition in the error message.
>
> I'm not entirely convinced by this change either:
>
> - if (equal || off1 != off2)
> + if (off2 > off1 + 1 || ((off2 == off1 + 1) && !equal))
>
> Passing probe_is_bound = true to partition_bound_bsearch() will I
> think cause it to return equal = false when the upper bound of one
> partition equals the lower bound of another, so relying on the "equal"
> flag here seems a bit dubious. I think I can just about convince
> myself that it works, but not for the reasons stated in the comments.
You are right. What's actually happening in the case where I was thinking
equal would be set to true is that off2 ends up being equal to off1, so
the second arm of that || is not checked at all.
> It also seems unnecessary for this code to be doing 2 binary searches.
> I think a better simplification would be to just do one binary search
> to find the gap that the lower bound fits in, and then test the upper
> bound of the new partition against the lower bound of the next
> partition (if there is one), as in the attached patch.
I agree. The patch looks good to me.
Thanks again.
Regards,
Amit