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

On Fri, Jun 10, 2011 at 6:16 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> 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.

Andrew - in case it's not clear, we're waiting on you to respond to
Hitoshi's comments or provide an updated patch.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company