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

[Teodor Sigaev]

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: teo...@sigaev.ru
                                                   WWW: http://www.sigaev.ru/

q4d-2.patch.gz
Description: GNU Zip compressed data

traversalValue-2.patch.gz
Description: GNU Zip compressed data

range-1.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers