Postgres Pro
Downloads
Home   >   mailing lists

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

From Matheus de Oliveira
Subject Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar andanyrange types
Date
Msg-id CAJghg4KOJvNRhf=LZR8OsZE5vDQqSiLDxx5o6baXe2gE6U0thw@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar andanyrange types  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
Responses Re: [PATCH] btree_gin, add support for uuid, bool, name, bpchar andanyrange types  (Tomas Vondra <tomas.vondra@2ndquadrant.com>)
List pgsql-hackers
Tree view
Hi all.

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

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,

pgsql-hackers by date:

Previous
From: Ildar Musin
Date:
Subject: Re: Failed to request an autovacuum work-item in silence
Next
From: Amit Kapila
Date:
Subject: Re: [HACKERS] Restrict concurrent update/delete with UPDATE ofpartition key