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