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

Hi all.

Em 4 de mar de 2018 16:00, "Tomas Vondra" <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.

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

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.

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?

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 ;)

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.

Thanks a lot.

Best regards,