Thread: [PATCH] btree_gin, add support for uuid, bool, name, bpchar andanyrange 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

On 03/08/2018 10:20 AM, Matheus de Oliveira wrote:
> Hi all.
> 
> Em 4 de mar de 2018 16:00, "Tomas Vondra" <tomas.vondra@2ndquadrant.com
> <mailto:tomas.vondra@2ndquadrant.com>> escreveu:
> 
> 
>     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.
> 
> 
> I pondered that either, and I also haven't thought about a good use
> case, but since it has B-Tree support, I thought it should be included
> on btree_gin as well, so I did.
> 

AFAIK having a B-tree opclass has other important implications (it kinda
determines important operators etc.), so it may not really mean B-tree
indexes on ranges are somewhat practical.

> If you all decide to remove, I'm totally fine with that.
> 

Not sure, but I'd probably cut it - adding opclasses in the future seems
less problematic than removing them.

> 
> 
>     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.
> 
> 
> I forgot I have changed that, sorry. I think though that 10.0 was a
> typo, since it has been introduced way before PostgreSQL 10. But you are
> right, it should be 1.1.
> 
> 
>     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.
> 
> 
> I can send it later as a bugfix then. Sounds better indeed.
> 

Just split the patch in two, and keep it.

> 
> 
>     3) The documentation refers to <type>range</type>, which is bogus as
>     there is no such type. It should say <type>anyrange</type> instead.
> 
> 
> I've just followed what has been done for ENUM type, if we are going to
> change for range we should also change to use anyenum, no?
> 

Hmmm, you're right the docs use <type>enum</type> on a couple of places.
But I see there's not a single mention of <type>range</type> but quite a
few references to <type>anyrange</type>. I'm not sure why exactly, but
I'm sure there's a reason.

> 
> 
>     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.
> 
> 
> Ok.
> 
> 
>     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).
> 
> 
> My English skills aren't very good, so feel free to tweak any comment or
> documentation I have done ;)
> 

Sure, ultimately someone else will do a final check.

> 
>     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).
> 
> 
> I really appreciate your review. I'd like to know what you think about
> my comments above.
> 

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Matheus,

Do you plan to post an updated version of the patch, of what is your
response to the points raised last week?

I still haven't made my mind regarding usefulness of range opclasses, so
I suggest to split the patch into two parts - 0001 for the opclasses
we're confident are useful, and 0002 for those extras. The committer
then may apply either 0001 or 0001+0002, depending on his judgment.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03/30/2018 10:51 PM, Matheus de Oliveira wrote:
> Hi all.
> 
> On Wed, Mar 21, 2018 at 1:47 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:
> 
> 
>     Do you plan to post an updated version of the patch, of what is your
>     response to the points raised last week?
> 
> 
> Very sorry about the long delay. I've been in a long trip, no time to
> look that carefully.
>  
> 
>     I still haven't made my mind regarding usefulness of range opclasses, so
>     I suggest to split the patch into two parts - 0001 for the opclasses
>     we're confident are useful, and 0002 for those extras. The committer
>     then may apply either 0001 or 0001+0002, depending on his judgment.
> 
> 
> I liked the idea. So, follows the patches:
> - 0001-btree_gin-uuid--base.v2.patch - all types but anyrange, and with
> the adjustments on comments you proposed
> - 0002-btree_gin-uuid--anyrange.v2.patch - adding the anyrange type
> (must be applied after 0001)
> 
> Anything else missing?
> 

I don't think so. I've marked it as RFC, but I have no idea if anyone is
going to push it by the end of this commitfest.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

somehow you missed some parts in 0001 patch, at least regression tests fail:

   CREATE EXTENSION btree_gin;
+ ERROR:  could not find function "gin_extract_value_uuid" in file 
"/usr/local/pgsql/lib/btree_gin.so"


Matheus de Oliveira wrote:
> Hi all.
> 
> On Wed, Mar 21, 2018 at 1:47 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com 
> <mailto:tomas.vondra@2ndquadrant.com>> wrote:
> 
> 
>     Do you plan to post an updated version of the patch, of what is your
>     response to the points raised last week?
> 
> 
> Very sorry about the long delay. I've been in a long trip, no time to look that 
> carefully.
> 
>     I still haven't made my mind regarding usefulness of range opclasses, so
>     I suggest to split the patch into two parts - 0001 for the opclasses
>     we're confident are useful, and 0002 for those extras. The committer
>     then may apply either 0001 or 0001+0002, depending on his judgment.
> 
> 
> I liked the idea. So, follows the patches:
> - 0001-btree_gin-uuid--base.v2.patch - all types but anyrange, and with the 
> adjustments on comments you proposed
> - 0002-btree_gin-uuid--anyrange.v2.patch - adding the anyrange type (must be 
> applied after 0001)
> 
> Anything else missing?
> 
> Best regards,
> -- 
> Matheus de Oliveira
> 
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Damn, I should have noticed that during the last review, but I missed
that somehow. Not sure Matheus will have time to look into it, so here
is a (hopefully) fixed version.

regards

On 4/5/18 1:16 PM, Teodor Sigaev wrote:
> somehow you missed some parts in 0001 patch, at least regression tests
> fail:
> 
>   CREATE EXTENSION btree_gin;
> + ERROR:  could not find function "gin_extract_value_uuid" in file
> "/usr/local/pgsql/lib/btree_gin.so"
> 
> 

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thanks to everyone, first patch is pushed.

Range opclass seems unusable because comparing function is close to dummy and 
BTree opclass is only useful to implement unique check constraint. So, for range 
it should different index structure to be useful.

Matheus de Oliveira wrote:
> 
> 
> On Thu, Apr 5, 2018 at 8:16 AM, Teodor Sigaev <teodor@sigaev.ru 
> <mailto:teodor@sigaev.ru>> wrote:
> 
>     somehow you missed some parts in 0001 patch, at least regression tests fail:
> 
>        CREATE EXTENSION btree_gin;
>     + ERROR:  could not find function "gin_extract_value_uuid" in file
>     "/usr/local/pgsql/lib/btree_gin.so"
> 
> 
> Ouch... My fault, filterdiff is acting weird for some reason...
> 
> Here is the corrected versions... I tested here applying on a clean master just 
> to be sure, all looks good.
> 
> Very sorry about that mess. I hope it can get in v11, it is a patch so simple, 
> but so useful for many people.
> 
> Best regards,
> -- 
> Matheus de Oliveira
> 
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/