Re: [PATCH] we have added support for box type in SP-GiST index

>> + * boxtype_spgist.c
> The names on the file header need to be changed, too.
Oops. fixed

>> I'll try to explain with two-dimensional example over points. ASCII-art:
> Thank you for the explanation. Should we incorporate this with the patch.
added

>> + cmp_double(const double a, const double b)
> Does this function necessary? We can just compare the doubles.
Yes, it compares with limited precision as it does by geometry operations.
Renamed to point actual arguments.

>
>> + boxPointerToRangeBox(BOX *box, RangeBox * rectangle)
> The "rectangle" variable in here should be renamed.
fixed

>
>> + Assert(is_infinite(b) == 0);
> This is failing on my test. You can reproduce with the script I have sent.
I didn't know:
# select '(1,inf)'::point;
point
---------
(1,inf)

fixed

>
>>> Wouldn't it be simpler to palloc and return the value on those functions?
>> evalRangeBox() initializes part of RectBox, so, it could not palloc its
>> result.
>> Other methods use the same signature just for consistency.
>
> I couldn't understand it. evalRangeBox() can palloc and return the
> result. evalRectBox() can return the result palloc'ed by
> evalRangeBox(). The caller wouldn't need to palloc anything.
evalRangeBox() is used to initialize fields of RangeBox in evalRectBox(). If
evalRangeBox() will palloc its result then we need to copy its result into
RangeBox struct and free. Let both fucntion use the same interface.

> I went through all other variables:
>> + int r = is_infinite(a);
>> + double x = *(double *) a;
>> + double y = *(double *) b;
>> + spgInnerConsistentIn *in = (spgInnerConsistentIn *) PG_GETARG_POINTER(0);
>> + spgLeafConsistentIn *in = (spgLeafConsistentIn *) PG_GETARG_POINTER(0);
>> + BOX *leafBox = DatumGetBoxP(in->leafDatum);
> Shouldn't they be "const", too?
They could. But it doesn't required. To be in consistent state I've removed
const modifier where possible

>
>>>> + /*
>>>> + * Begin. This block evaluates the median of coordinates of boxes
>>>> + */
>>>
>>> I would rather explain what the function does on the function header.
>>
>> fixed
> The "end" part of it is still there.
Oops again, fixed

>
>>> Do we really need to copy the traversalValues on allTheSame case.
>>> Wouldn't it work if just the same value is passed for all of them.
>>> The search shouldn't continue after allTheSame case.
>>
>> Seems, yes. spgist tree could contain a locng branches with allTheSame.
>
> Does SP-GiST allows any node under allTheSame to not being allTheSame?
No, SP-GiST doesn't allow that
> Not setting traversalValues for allTheSame worked fine with my test.
it works until allthesame branch contains only one inner node. Otherwise
traversalValue will be freeed several times, see spgWalk().
# select i as id, '1,2,3,4'::box as b into x from generate_series(1,1000000) i;
# create index ix on i using spgist (b);
# select count(*) from x where b && '1,2,3,4'::box; -- coredump
gdb:
#0 0x000000080143564a in thr_kill () from /lib/libc.so.7
#1 0x0000000801435636 in raise () from /lib/libc.so.7
#2 0x00000008014355b9 in abort () from /lib/libc.so.7
#3 0x0000000000a80739 in in_error_recursion_trouble () at elog.c:199
#4 0x0000000000abb748 in pfree (pointer=0x801e90868) at mcxt.c:1016
#5 0x000000000053330c in freeScanStackEntry (so=0x801e8d358,
stackEntry=0x801e935d8) at spgscan.c:47
#6 0x0000000000532cdb in spgWalk (index=0x801f1c588, so=0x801e8d358,
scanWholeIndex=1 '\001', storeRes=0x532d10 <storeBitm
...

>
>> + if (in->allTheSame)
> Most of the things happening before this check is not used in there.
> Shouldn't we move this to the top of the function?
yep, fixed

>
>> + out->nodeNumbers = (int *) palloc(sizeof(int) * in->nNodes);
> This could go before allTheSame block.
yep, fixed

I've attached all patches again. Thank you very much!

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/