On Fri, Jun 10, 2011 at 6:16 AM, Hitoshi Harada <umi.tanuki@gmail.com> wrote:
> 2011/2/24 Andrew Tipton <andrew.t.tipton@gmail.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
