On Fri, Jun 10, 2011 at 22:16, Hitoshi Harada <[email protected]> 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
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 ( 2593 603 603 7 2584 ));
+ 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 ));
DATA(insert ( 2594 604 604 1 2585 ));
DATA(insert ( 2594 604 604 2 2583 ));
DATA(insert ( 2594 604 604 3 2586 ));
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 3c183ce..84d1205 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DATA(insert OID = 2588 ( circle_overabo
*** 3768,3773 ****
--- 3768,3775 ----
/* support functions for GiST r-tree emulation */
DATA(insert OID = 2578 ( gist_box_consistent PGNSP PGUID 12 1 0 0 f f f t f i 5 0 16 "2281 603 23 26 2281" _null_ _null_ _null_ _null_ gist_box_consistent _null_ _null_ _null_ ));
DESCR("GiST support");
+ 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_ ));
+ DESCR("GiST support");
DATA(insert OID = 2579 ( gist_box_compress PGNSP PGUID 12 1 0 0 f f f t f i 1 0 2281 "2281" _null_ _null_ _null_ _null_ gist_box_compress _null_ _null_ _null_ ));
DESCR("GiST support");
DATA(insert OID = 2580 ( gist_box_decompress PGNSP PGUID 12 1 0 0 f f f t f i 1 0 2281 "2281" _null_ _null_ _null_ _null_ gist_box_decompress _null_ _null_ _null_ ));
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
