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(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/