Horton

Thread

Commits

  1. Document BRIN a bit more thoroughly

  1. create opclass documentation outdated

    Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-02-25T20:13:02Z

    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

  2. Re: create opclass documentation outdated

    Vik Fearing <vik@2ndquadrant.fr> — 2016-02-25T22:12:13Z

    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

  3. Re: create opclass documentation outdated

    Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-02-25T22:55:44Z

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

  4. Re: create opclass documentation outdated

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-03-01T01:06:59Z

    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

  5. Re: create opclass documentation outdated

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-03-02T15:53:42Z

    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

  6. Re: create opclass documentation outdated

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-03-02T18:21:07Z

    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

  7. Re: create opclass documentation outdated

    Emre Hasegeli <emre@hasegeli.com> — 2016-03-10T10:27:06Z

    >> 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.

  8. Re: create opclass documentation outdated

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-03-10T15:51:56Z

    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

  9. Re: create opclass documentation outdated

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-03-10T16:02:45Z

    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