Re: [PATCH]: fix bug in SP-GiST box_ops

At Fri, 10 Mar 2017 12:15:52 +0300, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote in <48f6934b-b994-4aa2-b6ad-aaa4f5a12877(at)postgrespro(dot)ru>
> On 10.03.2017 02:13, Tels wrote:
>
> > I can't comment on the code, but the grammar on the comments caught my
> > eye:
> >> +/* Can any range from range_box does not extend higher than this
> >> argument? */
> >> +static bool
> >> +overLower2D(RangeBox *range_box, Range *query)
> >> +{
> >> + return FPle(range_box->left.low, query->high) &&
> >> + FPle(range_box->right.low, query->high);
> >> +}
> > The sentence sounds quite garbled in English. I'm not entirely sure
> > what
> > it should be, but given the comment below "/* Can any range from
> > range_box
> > to be higher than this argument? */" maybe something like:
> >
> > /* Does any range from range_box extend to the right side of the
> > query? */
> >
> > If used, an analog wording should be used for overHigher2D's comment
> > like:
> >
> > /* Does any range from range_box extend to the left side of the query?
> > */
>
> I've changed comments as you proposed, but I've added missing "not"
> and left "Can":
>
> /* Can any range from range_box not extend to the right side of the
> query? */
> /* Can any range from range_box not extend to the left side of the
> query? */
>
> See also comments on overhigher and overlower operators from
> documentation:
>
> &< Does not extend to the right of?
> &> Does not extend to the left of?
>
> > Another question: Does it make sense to add the "minimal bad example
> > for
> > the '&<' case" as test case, too? After all, it should pass the test
> > after
> > the patch.
>
> Yes, it will make sense, but the Kyotaro's test case doesn't work for
> me and
> I still don't know how to force SP-GiST to create inner leaves without
> inserting hundreds of rows.

I admit that the case is quite unstable, or environment-dependent
and the minimal bad case no longer replays even for me. On the
other hand, inserting some hundreds of boxes makes it more stable
than only one box. I'm not sure that it is enough but it seems to
be the best we can. As I mentioned previously, box cannot be used
as the key for outer join so comparing one-by-one is out of hand
of regression test.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center