Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Hitoshi Harada]

2011/6/19 Hitoshi Harada umi.tan...@gmail.com:
 2011/6/17 Andrew Tipton andrew.t.tip...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Hitoshi Harada]

2011/6/17 Andrew Tipton andrew.t.tip...@gmail.com:
 On Fri, Jun 10, 2011 at 22:16, Hitoshi Harada umi.tan...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Andrew Tipton]

On Fri, Jun 10, 2011 at 22:16, Hitoshi Harada umi.tan...@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_consistentPGNSP 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
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index 43c4b12..ac4fb7f 100644
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
*** gist_box_consistent(PG_FUNCTION_ARGS)
*** 110,115 
--- 110,155 
   strategy));
  }
  
+ /* 
+  * GiST consistent function for traversing a BOX index using a POINT query.
+  */
+ Datum
+ gist_boxpoint_consistent(PG_FUNCTION_ARGS)
+ {
+ 	GISTENTRY  *entry = (GISTENTRY *) PG_GETARG_POINTER(0);
+ 	Point  *query = PG_GETARG_POINT_P(1);
+ 	BOX query_box;
+ 	StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2);
+ 
+ 	/* Oid		subtype = PG_GETARG_OID(3); */
+ 	bool	   *recheck = (bool *) PG_GETARG_POINTER(4);
+ 
+ 	/* All cases served by this function are exact */
+ 	*recheck = false;
+ 
+ 	elog(NOTICE, gist_boxpoint_consistent() called);
+ 
+ 	if (DatumGetBoxP(entry-key) == NULL || query == NULL)
+ 		PG_RETURN_BOOL(FALSE);
+ 
+ 	/* Turn our POINT query into a BOX. */
+ 	query_box.low = *query;
+ 	query_box.high = *query;
+ 
+ 	/*
+ 	 * if entry is not leaf, use rtree_internal_consistent, else use
+ 	 * gist_box_leaf_consistent
+ 	 */
+ 	if (GIST_LEAF(entry))
+ 		PG_RETURN_BOOL(gist_box_leaf_consistent(DatumGetBoxP(entry-key),
+ query_box,
+ strategy));
+ 	else
+ 		PG_RETURN_BOOL(rtree_internal_consistent(DatumGetBoxP(entry-key),
+  query_box,
+  strategy));
+ }
+ 
  static void
  adjustBox(BOX *b, BOX *addon)
  {
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index 3b88c41..f3645d5 100644
*** a/src/include/catalog/pg_amop.h
--- b/src/include/catalog/pg_amop.h
*** DATA(insert (	2593   603 603 4 s	495 783
*** 588,593 
--- 588,594 
  DATA(insert (	2593   603 603 5 s	496 783 0 ));
  DATA(insert (	2593   603 603 6 s	499 783 0 ));
  DATA(insert (	2593   603 603 7 s	498 783 0 ));
+ DATA(insert (	2593   603 600 7 s	433 783 0 ));
  DATA(insert (	2593   603 603 8 s	497 783 0 ));
  DATA(insert (	2593   603 603 9 s	2571 783 0 ));
  DATA(insert (	2593   603 603 10 s 2570 783 0 ));
diff --git a/src/include/catalog/pg_amproc.h b/src/include/catalog/pg_amproc.h
index 9e2da2c..bdfc57d 100644
*** a/src/include/catalog/pg_amproc.h
--- b/src/include/catalog/pg_amproc.h
*** DATA(insert (	2593   603 603 4 2580 ));
*** 170,175 
--- 170,182 
  DATA(insert (	2593   603 603 5 2581 ));
  DATA(insert (	2593   603 603 6 2582 ));
  DATA(insert (	

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Tom Lane]

Andrew Tipton andrew.t.tip...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Robert Haas]

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

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Hitoshi Harada]

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.


Regards,


-- 
Hitoshi Harada

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Patch: add GiST support for BOX @ POINT queries

[Andrew Tipton]

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
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index 86a5d90..a2c6cb6 100644
*** a/src/backend/access/gist/gistproc.c
--- b/src/backend/access/gist/gistproc.c
*** gist_box_consistent(PG_FUNCTION_ARGS)
*** 96,101 
--- 96,113 
  	if (DatumGetBoxP(entry-key) == NULL || query == NULL)
  		PG_RETURN_BOOL(FALSE);
  
+ 	if (strategy == 27)
+ 	{
+ 	/* Convert BOX @ POINT to the equivalent BOX @ BOX query */
+ 	Point *q_point = PG_GETARG_POINT_P(1);
+ 	BOX q_box;
+ 
+ 		q_box.low = *q_point;
+ 		q_box.high = *q_point;
+ 		query = q_box;
+ 		strategy = 7;   /* Strategy number for BOX @ BOX */
+ 	}
+ 
  	/*
  	 * if entry is not leaf, use rtree_internal_consistent, else use
  	 * gist_box_leaf_consistent
diff --git a/src/include/catalog/pg_amop.h b/src/include/catalog/pg_amop.h
index aabb900..eb03255 100644
*** a/src/include/catalog/pg_amop.h
--- b/src/include/catalog/pg_amop.h
*** DATA(insert (	2593   603 603 11 s 2573 7
*** 595,600 
--- 595,601 
  DATA(insert (	2593   603 603 12 s 2572 783 0 ));
  DATA(insert (	2593   603 603 13 s 2863 783 0 ));
  DATA(insert (	2593   603 603 14 s 2862 783 0 ));
+ DATA(insert (	2593   603 600 27 s 433 783 0 ));
  
  /*
   * gist point_ops

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Patch: add GiST support for BOX @ POINT queries

[Kevin Grittner]

Andrew Tipton andrew.t.tip...@gmail.com wrote:
 
 should I add this to CF-Next?
 
Yes.
 
-Kevin

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers