From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, amul sul <sulamul(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Multi column range partition table |
Date: | 2017-07-03 02:32:15 |
Message-ID: | 75faa42a-24c7-08f0-e549-9c3c271c6da6@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)lab(dot)ntt(dot)co(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Craig Ringer | 2017-07-03 02:58:53 | Re: [HACKERS] Segmentation fault in libpq |
Previous Message | Masahiko Sawada | 2017-07-03 02:31:18 | Re: Get stuck when dropping a subscription during synchronizing table |