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

From: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
To: Matheus de Oliveira <matioli(dot)matheus(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar and anyrange types
Date: 2018-03-04 15:00:20
Message-ID: 63ac7267-b0de-d7cb-9528-2e1e19d693b9@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
btree_gin-fixes.patch text/x-patch 3.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2018-03-04 15:05:59 Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?
Previous Message Stephen Frost 2018-03-04 14:49:18 Re: [PATCH] Verify Checksums during Basebackups