Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar and anyrange types

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