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...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
