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

> Do reconstructedValues is required now?  Wouldn't it be cleaner to use
> the new field on the prefix tree implementation, too?
reconstructedValues is needed to reconctruct full indexed value and should match
to type info indexed column

>
> We haven't had specific memory context for reconstructedValues.  Why
> is it required for the new field?
because pg knows type of reconstructedValues (see above why) but traversalValue
just a void*, SP-GiST core doesn't knnow how to free memory of allocated for it.


>
>> +       Memory for <structfield>traversalValues</> should be allocated in
>> +       the default context, but make sure to switch to
>> +       <structfield>traversalMemoryContext</> before allocating memory
>> +       for pointers themselves.
>
> This sentence is not understandable.  Shouldn't it say "the memory
> should *not* be allocated in the default context"?  What are the
> "pointers themselves"?
Clarified, I hope


>
> The box opclass is broken because of the floating point arithmetics of
> the box type.  You can reproduce it with the attached script.  We
hope, fixed. At least, seems, syncronized with seqscan

> really need to fix the geometric types, before building more on them.
> They fail to serve the only purpose they are useful for, demonstrating
> features.
agree, but it's not a deal for this patch

>
> It think the opclass should support the cross data type operators like
> box @> point.  Ideally we should do it by using multiple opclasses in
> an opfamily.  The floating problem will bite us again in here, because
> some of the operators are not using FP macros.
Again, agree. But I suggest to do it by separate patch.

>
> The tests check not supported operator "@".  It should be "<@".  It is
> nice that the opclass doesn't support long deprecated operators.
fixed


>> + #define LT      -1
>> + #define GT       1
>> + #define EQ       0
>
> Using these numbers is a very well established pattern.  I don't think
> macros make the code any more readable.
fixed

>
>> + /* SP-GiST API functions */
>> + Datum       spg_box_quad_config(PG_FUNCTION_ARGS);
>> + Datum       spg_box_quad_choose(PG_FUNCTION_ARGS);
>> + Datum       spg_box_quad_picksplit(PG_FUNCTION_ARGS);
>> + Datum       spg_box_quad_inner_consistent(PG_FUNCTION_ARGS);
>> + Datum       spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS);
>
> I guess they should go to the header file.
Remove them because they are presented in auto-generated file
./src/include/utils/builtins.h

range patch is unchanged, but still attached to keep them altogether.

--
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/