Re: Patch: add GiST support for BOX @> POINT queries

2011/2/24 Andrew Tipton <andrew(dot)t(dot)tipton(at)gmail(dot)com>:
> While playing around with the BOX and POINT datatypes, I was surprised to
> note that BOX @> POINT (and likewise POINT <@ BOX) queries were not using
> the GiST index I had created on the BOX column.  The attached patch adds a
> new strategy @>(BOX,POINT) to the box_ops opclass.  Internally,
> gist_box_consistent simply transforms the POINT into its corresponding BOX.
> This is my first Postgres patch, and I wasn't able to figure out how to go
> about creating a regression test for this change.  (All existing tests do
> pass, but none of them seem to specifically test index behaviour.)

I reviewed the patch and worried about hard-wired magic number as
StrategyNumber. At least you should use #define to indicate the
number's meaning.

In addition, the modified gist_box_consistent() is too dangerous;
q_box is declared in the if block locally and is referenced, which
pointer is passed to the outer process of the block. AFAIK if the
local memory of each block is alive outside if block is
platform-dependent.

Isn't it worth adding new consistent function for those purposes? The
approach in the patch as stands looks kludge to me.

Regards,

--
Hitoshi Harada