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

On 03/08/2018 10:20 AM, Matheus de Oliveira wrote:
> Hi all.
>
> Em 4 de mar de 2018 16:00, "Tomas Vondra" <tomas(dot)vondra(at)2ndquadrant(dot)com
> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)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