On Fri, Jun 10, 2011 at 6:16 AM, Hitoshi Harada <umi.tan...@gmail.com> wrote: > 2011/2/24 Andrew Tipton <andrew.t.tip...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
