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

Here are my first comments. I haven't read the actual index
implementation, yet.

I think traversal value is a useful addition. It is nice that the
implementation for the range types is also changed. My questions
about them are:

Do reconstructedValues is required now? Wouldn't it be cleaner to use
the new field on the prefix tree implementation, too?

We haven't had specific memory context for reconstructedValues. Why
is it required for the new field?

> + 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"?

The box opclass is broken because of the floating point arithmetics of
the box type. You can reproduce it with the attached script. We
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.

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.

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

We needs tests for the remaining operators and for adding new values
to the index. I am not sure create_index.sql is the best place for
them. Maybe we should use the test scripts of "box".

> + #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.

> + /* 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.