Thread
-
Patch: add GiST support for BOX @> POINT queries
Andrew Tipton <andrew.t.tipton@gmail.com> — 2011-02-23T23:05:40Z
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 know it is quite late in the CommitFest, should I add this to CF-Next? -Andrew
-
Re: Patch: add GiST support for BOX @> POINT queries
Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-02-23T23:12:56Z
Andrew Tipton <andrew.t.tipton@gmail.com> wrote: > should I add this to CF-Next? Yes. -Kevin
-
Re: Patch: add GiST support for BOX @> POINT queries
Hitoshi Harada <umi.tanuki@gmail.com> — 2011-06-10T10:16:57Z
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. Regards, -- Hitoshi Harada
-
Re: Patch: add GiST support for BOX @> POINT queries
Robert Haas <robertmhaas@gmail.com> — 2011-06-14T15:25:08Z
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
-
Re: Patch: add GiST support for BOX @> POINT queries
Andrew Tipton <andrew.t.tipton@gmail.com> — 2011-06-17T10:43:47Z
On Fri, Jun 10, 2011 at 22:16, Hitoshi Harada <umi.tanuki@gmail.com> wrote: > > 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. Thanks for your review. Coming back to this patch after a few months' time, I have to say it looks pretty hackish to my eyes as well. :) I've attempted to add a new consistent function, gist_boxpoint_consistent(), but the GiST subsystem doesn't call it -- it continues to call gist_box_consistent(). My very simple testcase is: CREATE TABLE test (key TEXT PRIMARY KEY, boundary BOX NOT NULL); CREATE INDEX ON test USING gist (boundary); INSERT INTO test VALUES ('a', '(2,2,5,5)'), ('b', '(4,4,8,8)'), ('c', '(7,7,11,11)'); SELECT * FROM test WHERE boundary @> '(4,4)'::POINT; Prior to my patch, this query is executed as a straightforward seqscan. Once I add a new strategy to pg_amop.h: + DATA(insert ( 2593 603 600 7 s 433 783 0 )); (603 is the BOX oid, 600 is the POINT oid, and 433 is the @> operator oid): ...the plan switches to an index scan and gist_box_consistent() is called; at this point, the query fails to return the correct results. But even after adding the new consistent proc to pg_proc.h: + DATA(insert OID = 8000 ( gist_boxpoint_consistent PGNSP PGUID 12 1 0 0 f f f t f i 5 0 16 "2281 600 23 26 2281" _null_ _null_ _null_ _null_ gist_boxpoint_consistent _null_ _null_ _null_ )); And adding it as a new support function in pg_amproc.h: + DATA(insert ( 2593 603 600 1 8000 )); + DATA(insert ( 2593 603 600 2 2583 )); + DATA(insert ( 2593 603 600 3 2579 )); + DATA(insert ( 2593 603 600 4 2580 )); + DATA(insert ( 2593 603 600 5 2581 )); + DATA(insert ( 2593 603 600 6 2582 )); + DATA(insert ( 2593 603 600 7 2584 )); ...my gist_boxpoint_consistent() function still doesn't get called. At this point I'm a bit lost -- while pg_amop.h has plenty of examples of crosstype comparison operators for btree index methods, there are none for GiST. Is GiST somehow a special case in this regard? -Andrew -
Re: Patch: add GiST support for BOX @> POINT queries
Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-17T13:59:40Z
Andrew Tipton <andrew.t.tipton@gmail.com> writes: > At this point I'm a bit lost -- while pg_amop.h has plenty of examples > of crosstype comparison operators for btree index methods, there are > none for GiST. Is GiST somehow a special case in this regard? AFAIR, GIST doesn't use the concept of a crosstype opclass entry. It only works with primary opclass entries. You have to set both amproclefttype and amprocrighttype to the datatype of the indexable column, regardless of what the other argument actually is. (I think this implies that you can't have more than one consistent function per opclass, which means you have to do whatever it is you have in mind by patching the existing consistent function, not adding another one alongside it.) regards, tom lane
-
Re: Patch: add GiST support for BOX @> POINT queries
Hitoshi Harada <umi.tanuki@gmail.com> — 2011-06-19T14:30:54Z
2011/6/17 Andrew Tipton <andrew.t.tipton@gmail.com>: > On Fri, Jun 10, 2011 at 22:16, Hitoshi Harada <umi.tanuki@gmail.com> wrote: >> >> Isn't it worth adding new consistent function for those purposes? The >> approach in the patch as stands looks kludge to me. > > Thanks for your review. Coming back to this patch after a few months' > time, I have to say it looks pretty hackish to my eyes as well. :) > > I've attempted to add a new consistent function, > gist_boxpoint_consistent(), but the GiST subsystem doesn't call it -- > it continues to call gist_box_consistent(). My very simple testcase > is: > > CREATE TABLE test (key TEXT PRIMARY KEY, boundary BOX NOT NULL); > CREATE INDEX ON test USING gist (boundary); > INSERT INTO test VALUES ('a', '(2,2,5,5)'), ('b', '(4,4,8,8)'), ('c', > '(7,7,11,11)'); > SELECT * FROM test WHERE boundary @> '(4,4)'::POINT; > > Prior to my patch, this query is executed as a straightforward seqscan. > > Once I add a new strategy to pg_amop.h: > + DATA(insert ( 2593 603 600 7 s 433 783 0 )); > > (603 is the BOX oid, 600 is the POINT oid, and 433 is the @> operator oid): > ...the plan switches to an index scan and gist_box_consistent() is > called; at this point, the query fails to return the correct results. > > But even after adding the new consistent proc to pg_proc.h: > + DATA(insert OID = 8000 ( gist_boxpoint_consistent PGNSP PGUID 12 > 1 0 0 f f f t f i 5 0 16 "2281 600 23 26 2281" _null_ _null_ _null_ > _null_ gist_boxpoint_consistent _null_ _null_ _null_ )); > > And adding it as a new support function in pg_amproc.h: > + DATA(insert ( 2593 603 600 1 8000 )); > + DATA(insert ( 2593 603 600 2 2583 )); > + DATA(insert ( 2593 603 600 3 2579 )); > + DATA(insert ( 2593 603 600 4 2580 )); > + DATA(insert ( 2593 603 600 5 2581 )); > + DATA(insert ( 2593 603 600 6 2582 )); > + DATA(insert ( 2593 603 600 7 2584 )); > > ...my gist_boxpoint_consistent() function still doesn't get called. > > At this point I'm a bit lost -- while pg_amop.h has plenty of examples > of crosstype comparison operators for btree index methods, there are > none for GiST. Is GiST somehow a special case in this regard? It was I that was lost. As Tom mentioned, GiST indexes have records in pg_amop in their specialized way. I found gist_point_consistent has some kind of hack for that and pg_amop for point_ops records have multiple crosstype for that. So, if I understand correctly your first approach modifying gist_box_consistent was the right way, although trivial issues should be fixed. Also, you may want to follow point_ops when you are worried if the counterpart operator of commutator should be registered or not. Looking around those mechanisms, it occurred to me that you mentioned only box @> point. Why don't you add circly @> point, poly @> point as well as box? Is that hard? Regards, -- Hitoshi Harada -
Re: Patch: add GiST support for BOX @> POINT queries
Hitoshi Harada <umi.tanuki@gmail.com> — 2011-07-09T14:29:49Z
2011/6/19 Hitoshi Harada <umi.tanuki@gmail.com>: > 2011/6/17 Andrew Tipton <andrew.t.tipton@gmail.com>: >> >> At this point I'm a bit lost -- while pg_amop.h has plenty of examples >> of crosstype comparison operators for btree index methods, there are >> none for GiST. Is GiST somehow a special case in this regard? > > It was I that was lost. As Tom mentioned, GiST indexes have records in > pg_amop in their specialized way. I found gist_point_consistent has > some kind of hack for that and pg_amop for point_ops records have > multiple crosstype for that. So, if I understand correctly your first > approach modifying gist_box_consistent was the right way, although > trivial issues should be fixed. Also, you may want to follow point_ops > when you are worried if the counterpart operator of commutator should > be registered or not. > > Looking around those mechanisms, it occurred to me that you mentioned > only box @> point. Why don't you add circly @> point, poly @> point as > well as box? Is that hard? > It looks like the time to wrap up. I marked "Return with Feedback" on this patch, since response from author has not come for a while. You may think the fix was pretty easy and the patch be small, but more general approach was preferred, I guess. Looking forward to seeing it in better shape next time! Thanks, -- Hitoshi Harada