Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar andanyrange types - Mailing list pgsql-hackers

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