Emre Hasegeli wrote:
> >> In create_opclass.sgml, not only do we have the list of AMs supporting
> >> STORAGE, but there is also a paragraph describing which AMs do what
> >> for input datatypes of FUNCTION members (around line 175).
> >
> > I placed BRIN together with gist/gin/spgist here, namely that the optype
> > defaults to the opclass' datatype.
>
> Requiring STORAGE type for BRIN opclasses is more of a bug than a
> feature. None of the operator classes actually support storing values
> with different type. We had intended to support it for inclusion
> opclass to index points by casting them to box, but then ripped it out
> because of inconsistencies on the operators caused by floating point
> arithmetics.
>
> The problem is that the operator classes try to query the strategies
> using the datatype they got from TupleDesc structure. It fails at
> least for cidr, because it is casted implicitly and indexed as inet.
> All of the BRIN operator classes can fail the same way for user
> defined types. Adding STORAGE to all operator classes have seemed to
> me like an easy fix at the time I noticed this problem, but what we
> really need to do is to fix this than document. We should to make
> BRIN use the datatype of the operator class as btree does.
Hmm, but if we ever add support for other types in inclusion as you
describe, we will need STORAGE, no? I think it's unfortunate that the
code currently uses the field where it's not really necessary, but
harmless; if people break their systems by introducing bogus optypes,
it's their fault. We can discuss improving this in the future, but in
the back branches it seems fine to leave it as is.
One possible idea to improve this (not for 9.6!) is to use the new
opclass-checker mechanism recently introduced in 65c5fcd353a859da9e6;
for minmax we could check that the opclass is defined with optype set to
0 or a type to which there's an implicit cast -- or some similar rule.
Not sure what we need for inclusion, and I'm fairly certain that we
could need completely different rules for other types of opclasses;
consider the idea of a bloom filter which was floated early during BRIN
development, for example, where the stored type is completely different
from the indexed type.
Anyway I'm incluined to push the patch with that paragraph as originally
posted.
> Other than those, the changes look good to me.
Thanks for the review. I will push with those fixes.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
