Обсуждение: create opclass documentation outdated
Hello, I just saw that the CREATE OPERATOR CLASS documentation doesn't mention that BRIN indexes also support the STORAGE parameter. Patch attached. Regards -- Julien Rouhaud http://dalibo.com - http://dalibo.org
Вложения
On 02/25/2016 09:13 PM, Julien Rouhaud wrote: > Hello, > > I just saw that the CREATE OPERATOR CLASS documentation doesn't mention > that BRIN indexes also support the STORAGE parameter. Good catch! > Patch attached. I think this is the wrong approach; that parenthetical list now represents a full 50% of the available AMs. I submit it should be removed altogether. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 25/02/2016 23:12, Vik Fearing wrote: > On 02/25/2016 09:13 PM, Julien Rouhaud wrote: >> Hello, >> >> I just saw that the CREATE OPERATOR CLASS documentation doesn't >> mention that BRIN indexes also support the STORAGE parameter. > > Good catch! > Thanks >> Patch attached. > > I think this is the wrong approach; that parenthetical list now > represents a full 50% of the available AMs. I submit it should be > removed altogether. > With further inspection, the "Interfacing Extensions To Indexes" (§35.14) documentation also has a list of AMs supporting the STORAGE parameter. I believe having at least one page referencing which AMs support this parameter could be helpful for people who want to implement new opclass. I'll send an updated patch as soon we'll have a consensus here :) - -- Julien Rouhaud http://dalibo.com - http://dalibo.org -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (GNU/Linux) iQEcBAEBAgAGBQJWz4ZwAAoJELGaJ8vfEpOqijkIAJXzMwx2C6TzekrkWSpNK+qC eCXuoCQhcLh6Uw80UG8MCnDhubSqG5pJwvjOf9Kih4an1Z0b5lvHORtUMVvepE6T J8KxOw3s67y7eeXpKl2974l0Y+5ylgGWhjsaQOrKWu5bjYqoXQ+yGfWmUC81gY/C rIu1X8tUJyYYrNI5ka4+Q0PN7jf//g3aYhqJmOVWzyJDXjS+QIQpMifp/L0+9GXb yCb0jqlnwpngqSsyAtIPul/j04ZWPs387xQL0+mD0JFA9mtFo6iTg4SyUtOAsR7N ifgog0j2132z7wjSfm94mIhuVStfogy1pkW3BEkhfxlj4T3S1LxUZPjDEOKBMWQ= =aUeH -----END PGP SIGNATURE-----
Julien Rouhaud <julien.rouhaud@dalibo.com> writes:
> On 25/02/2016 23:12, Vik Fearing wrote:
>> On 02/25/2016 09:13 PM, Julien Rouhaud wrote:
>>> I just saw that the CREATE OPERATOR CLASS documentation doesn't
>>> mention that BRIN indexes also support the STORAGE parameter.
>> I think this is the wrong approach; that parenthetical list now
>> represents a full 50% of the available AMs. I submit it should be
>> removed altogether.
> With further inspection, the "Interfacing Extensions To Indexes"
> (§35.14) documentation also has a list of AMs supporting the STORAGE
> parameter. I believe having at least one page referencing which AMs
> support this parameter could be helpful for people who want to
> implement new opclass.
I think the problem here is more thoroughgoing, namely that neither
create_opclass.sgml nor xindex.sgml was touched at all for BRIN.
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).
xindex.sgml has that one list you mentioned, but there's not much
point in fixing that when BRIN indexes fail to be described either in
35.14.2 or 35.14.3, where they certainly should be. And there are
other places throughout that chapter that say that such-and-such AMs
support each feature as it's discussed.
I'm inclined to feel that the right answer is to fix all these
omissions, not to decide that we're not maintaining this documentation
anymore. Alvaro, I think this one's in your court.
regards, tom lane
Tom Lane wrote: > I'm inclined to feel that the right answer is to fix all these > omissions, not to decide that we're not maintaining this documentation > anymore. Alvaro, I think this one's in your court. Will fix. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tom Lane 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. > xindex.sgml has that one list you mentioned, but there's not much > point in fixing that when BRIN indexes fail to be described either in > 35.14.2 or 35.14.3, where they certainly should be. And there are > other places throughout that chapter that say that such-and-such AMs > support each feature as it's discussed. In 35.14.2 I added the table of strategies in the "minmax" opclasses; the list for the "inclusion" opclasses would be much longer. Since GIN doesn't document anything other than its array support, I'm not really sure that we want/need to document the "inclusion" opclasses also. In 35.14.3 I added the table of four basic support procs. I think that's all that's strictly necessary. For inclusion we have a bunch of additional support procs. I also added a paragraph to 35.14.5, indicating that "minmax" is pretty much the same as btree. "Inclusion" opclasses are supposed to support cross-datatype operators too (supposedly we could support above/below/containment etc for points relative to boxes, for example, if we got around to make a decision regarding floating point comparisons in geometry operators), but we don't have any, so I hesitate to say anything about this. Patch attached. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
>> 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. > + of each operator class interpret the strategy nnumbers according to the Typo: nnumbers. > + Operator classes based on the <literal>Inclusion</> framework can > + theoretically support cross-data-type operations, but there's no > + demonstrating implementation yet. This part of the inclusion class is not committed, so this paragraph shouldn't be there. Other than those, the changes look good to me.
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
Alvaro Herrera wrote: > 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. Hypothetical situation where a different storage type might be useful with minmax: you store int2 values containing ln() of a numeric column. (Not sure that there's any actual situation for people to store data where this is valuable; consider astronomical distances, for example.) You'd need to have support for a "cast" method that takes the values from the ScanKey and applies ln() to them before doing the comparisons, but it seems a reassonable setup. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services