Hi,
On 02/20/2018 03:34 PM, Matheus de Oliveira wrote:
> Hi all.
>
> Here is a patch to add support for more types on btree_gin.
>
> I was missing UUID type, so I added it. Since I was there, I checked
> all other built-in types with B-tree but not GIN support, and the
> remaining list was: uuid, bool, name, bpchar and anyrange (at least
> ones that seem to make sense to me). So I added support for all of
> them.
>
> If you have any other type I missed and you wish to have support to,
> please let me know and I can add it.
>
I've looked at this patch today - it's a fairly straightforward addition
to btree_gin, and it seems in pretty good shape in general. It passes
all the various tests (even under valgrind), and the code seems OK too.
A couple of minor comments:
1) I personally am not that sure GIN indexes on ranges are very useful,
considering those columns are usually queried for containment (i.e. is
this value contained in the range) rather than equality. And we already
have gist/spgist opclasses for ranges, which seems way more useful. We
seem to already have hash opclasses for ranges, but I'm not sure that's
a proof of usefulness.
So I'd cut this, although it's a tiny amount of code.
2) The patch tweaks a couple of .sql files from previous versions. It
modifies a comment in the 1.0--1.1 upgrade script from
-- macaddr8 datatype support new in 10.0.
to
-- macaddr8 datatype support new in 1.0.
which is obviously incorrect, because not only is that in upgrade script
to 1.1. (so it should be "new in 1.1) but the original comment probably
refers to PostgreSQL 10, not the btree_gin version.
It also tweaks \echo in 1.1--1.2 upgrade script to mention 1.2 instead
of 1.1. This change seems correct, but it seems more like a bugfix that
part of this patch.
3) The documentation refers to <type>range</type>, which is bogus as
there is no such type. It should say <type>anyrange</type> instead.
4) The opclass is called "anyrange_ops", which is somewhat inconsistent
with the opclasses for btree, hash, gist and spgist. All those index
types use "range_ops" so I suggest using the same name.
5) I've tweaked a comment in btree_gin.c a bit, the original wording
seemed a bit unclear to me. And I've moved part of the comment to the
following function (it wasn't really about the left-most value).
Attached is a patch that does all of this, but it may be incomplete (I
haven't really checked if it breaks tests, for example).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/contrib/btree_gin/btree_gin--1.0--1.1.sql b/contrib/btree_gin/btree_gin--1.0--1.1.sql
index 8965247..dd81d27 100644
--- a/contrib/btree_gin/btree_gin--1.0--1.1.sql
+++ b/contrib/btree_gin/btree_gin--1.0--1.1.sql
@@ -3,7 +3,7 @@
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "ALTER EXTENSION btree_gin UPDATE TO '1.1'" to load this file. \quit
--- macaddr8 datatype support new in 1.0.
+-- macaddr8 datatype support new in 10.0.
CREATE FUNCTION gin_extract_value_macaddr8(macaddr8, internal)
RETURNS internal
AS 'MODULE_PATHNAME'
diff --git a/contrib/btree_gin/btree_gin--1.2--1.3.sql b/contrib/btree_gin/btree_gin--1.2--1.3.sql
index f7523a3..fe80983 100644
--- a/contrib/btree_gin/btree_gin--1.2--1.3.sql
+++ b/contrib/btree_gin/btree_gin--1.2--1.3.sql
@@ -117,7 +117,7 @@ RETURNS internal
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT IMMUTABLE;
-CREATE OPERATOR CLASS anyrange_ops
+CREATE OPERATOR CLASS range_ops
DEFAULT FOR TYPE anyrange USING gin
AS
OPERATOR 1 <,
diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c
index e1206a6..b5b26c9 100644
--- a/contrib/btree_gin/btree_gin.c
+++ b/contrib/btree_gin/btree_gin.c
@@ -510,18 +510,20 @@ leftmostvalue_bool(void)
GIN_SUPPORT(bool, false, leftmostvalue_bool, btboolcmp)
/*
- * Use a similar trick to that used for numeric for anyrange, since we don't
- * know the concrete type. We could try to build a fake empty range, but that
- * seems weaker and more complex than simple using the NULL ref trick.
- *
- * Note that we use CallerFInfoFunctionCall2 here so that range_cmp
- * gets a valid fn_extra to work with. Unlike most other type comparison
- * routines it needs it, so we can't use DirectFunctionCall2.
+ * Similarly to numeric, we don't know the left-most value, although for
+ * different reasons (numeric does not have one, while for anyarray we
+ * don't even know the concrete type). We could try to build a fake empty
+ * range, but we simply use PointerGetDatum(NULL) just like for Numeric.
*/
#define ANYRANGE_IS_LEFTMOST(x) ((x) == NULL)
PG_FUNCTION_INFO_V1(gin_anyrange_cmp);
+/*
+ * Note that we use CallerFInfoFunctionCall2 here so that range_cmp
+ * gets a valid fn_extra to work with. Unlike most other type comparison
+ * routines it needs it, so we can't use DirectFunctionCall2.
+ */
Datum
gin_anyrange_cmp(PG_FUNCTION_ARGS)
{
diff --git a/doc/src/sgml/btree-gin.sgml b/doc/src/sgml/btree-gin.sgml
index a951f86..e1315da 100644
--- a/doc/src/sgml/btree-gin.sgml
+++ b/doc/src/sgml/btree-gin.sgml
@@ -18,7 +18,7 @@
<type>varchar</type>, <type>text</type>, <type>bytea</type>, <type>bit</type>,
<type>varbit</type>, <type>macaddr</type>, <type>macaddr8</type>, <type>inet</type>,
<type>cidr</type>, <type>uuid</type>, <type>name</type>, <type>bool</type>,
- <type>bpchar</type>, and all <type>enum</type> and <type>range</type> types.
+ <type>bpchar</type>, and all <type>enum</type> and <type>anyrange</type> types.
</para>
<para>
