pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Teodor Sigaev
CREATE INDEX ... INCLUDING (column[, ...])

Now indexes (but only B-tree for now) can contain "extra" column(s) which
doesn't participate in index structure, they are just stored in leaf
tuples. It allows to use index only scan by using single index instead
of two or more indexes.

Author: Anastasia Lubennikova with minor editorializing by me
Reviewers: David Rowley, Peter Geoghegan, Jeff Janes

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/386e3d7609c49505e079c40c65919d99feb82505

Modified Files
--------------
contrib/dblink/dblink.c                       |  26 +--
contrib/tcn/tcn.c                             |   6 +-
doc/src/sgml/catalogs.sgml                    |   8 +
doc/src/sgml/indexam.sgml                     |   5 +-
doc/src/sgml/indices.sgml                     |   7 +-
doc/src/sgml/ref/create_index.sgml            |  41 +++-
doc/src/sgml/ref/create_table.sgml            |  36 ++-
src/backend/access/brin/brin.c                |   1 +
src/backend/access/common/indextuple.c        |  31 +++
src/backend/access/gin/ginutil.c              |   1 +
src/backend/access/gist/gist.c                |   1 +
src/backend/access/hash/hash.c                |   1 +
src/backend/access/index/genam.c              |  16 +-
src/backend/access/nbtree/nbtinsert.c         |  45 +++-
src/backend/access/nbtree/nbtpage.c           |   5 +-
src/backend/access/nbtree/nbtree.c            |   1 +
src/backend/access/nbtree/nbtsearch.c         |   2 +
src/backend/access/nbtree/nbtsort.c           |  48 +++-
src/backend/access/nbtree/nbtutils.c          |  25 ++-
src/backend/access/spgist/spgutils.c          |   1 +
src/backend/bootstrap/bootparse.y             |   2 +
src/backend/bootstrap/bootstrap.c             |   2 +-
src/backend/catalog/heap.c                    |   3 +-
src/backend/catalog/index.c                   |  45 ++--
src/backend/catalog/indexing.c                |   1 +
src/backend/catalog/pg_constraint.c           |  26 ++-
src/backend/catalog/toasting.c                |   1 +
src/backend/commands/indexcmds.c              |  60 +++--
src/backend/commands/matview.c                |   6 +-
src/backend/commands/tablecmds.c              |   9 +-
src/backend/commands/trigger.c                |   1 +
src/backend/commands/typecmds.c               |   1 +
src/backend/executor/execIndexing.c           |  14 +-
src/backend/executor/nodeIndexscan.c          |   8 +-
src/backend/nodes/copyfuncs.c                 |   2 +
src/backend/nodes/equalfuncs.c                |   2 +
src/backend/nodes/outfuncs.c                  |   3 +
src/backend/optimizer/path/indxpath.c         |   2 +-
src/backend/optimizer/path/pathkeys.c         |   7 +
src/backend/optimizer/util/plancat.c          |  32 +--
src/backend/parser/analyze.c                  |   6 +-
src/backend/parser/gram.y                     |  57 +++--
src/backend/parser/parse_relation.c           |   2 +-
src/backend/parser/parse_target.c             |   2 +-
src/backend/parser/parse_utilcmd.c            | 121 +++++++++--
src/backend/utils/adt/ruleutils.c             |  32 +++
src/backend/utils/adt/selfuncs.c              |   4 +-
src/backend/utils/cache/relcache.c            |  83 ++++---
src/backend/utils/sort/tuplesort.c            |   5 +-
src/bin/pg_dump/pg_dump.c                     |  65 +++++-
src/bin/pg_dump/pg_dump.h                     |   6 +-
src/include/access/amapi.h                    |   2 +
src/include/access/itup.h                     |   2 +
src/include/access/nbtree.h                   |   3 +-
src/include/catalog/catversion.h              |   2 +-
src/include/catalog/pg_constraint.h           |  23 +-
src/include/catalog/pg_constraint_fn.h        |  21 +-
src/include/catalog/pg_index.h                |  38 ++--
src/include/nodes/execnodes.h                 |   9 +-
src/include/nodes/parsenodes.h                |   5 +-
src/include/nodes/relation.h                  |  13 +-
src/include/utils/rel.h                       |  16 +-
src/test/regress/expected/create_index.out    |  19 ++
src/test/regress/expected/index_including.out | 301 ++++++++++++++++++++++++++
src/test/regress/parallel_schedule            |   2 +-
src/test/regress/serial_schedule              |   1 +
src/test/regress/sql/create_index.sql         |  20 ++
src/test/regress/sql/index_including.sql      | 181 ++++++++++++++++
68 files changed, 1320 insertions(+), 255 deletions(-)


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Tom Lane-2
Teodor Sigaev <[hidden email]> writes:
> CREATE INDEX ... INCLUDING (column[, ...])

Buildfarm members that don't like // comments are dying on this bit
in tuplesort.c:

        state->nKeys = IndexRelationGetNumberOfKeyAttributes(indexRel); //FIXME

I assume that the problem here is larger than just failure to adhere to
C89 comment style.  Was this patch really ready to commit?  I'm not very
happy that such a large patch went from "Needs review" to "Committed" in
the blink of an eye on the very last commitfest day ... and artifacts like
this aren't doing anything to increase my confidence in it.

                        regards, tom lane


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Robert Haas
On Fri, Apr 8, 2016 at 2:29 PM, Tom Lane <[hidden email]> wrote:

> Teodor Sigaev <[hidden email]> writes:
>> CREATE INDEX ... INCLUDING (column[, ...])
>
> Buildfarm members that don't like // comments are dying on this bit
> in tuplesort.c:
>
>         state->nKeys = IndexRelationGetNumberOfKeyAttributes(indexRel); //FIXME
>
> I assume that the problem here is larger than just failure to adhere to
> C89 comment style.  Was this patch really ready to commit?  I'm not very
> happy that such a large patch went from "Needs review" to "Committed" in
> the blink of an eye on the very last commitfest day ... and artifacts like
> this aren't doing anything to increase my confidence in it.

+1.  I wonder if this should be reverted entirely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Stephen Frost
* Robert Haas ([hidden email]) wrote:

> On Fri, Apr 8, 2016 at 2:29 PM, Tom Lane <[hidden email]> wrote:
> > Teodor Sigaev <[hidden email]> writes:
> >> CREATE INDEX ... INCLUDING (column[, ...])
> >
> > Buildfarm members that don't like // comments are dying on this bit
> > in tuplesort.c:
> >
> >         state->nKeys = IndexRelationGetNumberOfKeyAttributes(indexRel); //FIXME
> >
> > I assume that the problem here is larger than just failure to adhere to
> > C89 comment style.  Was this patch really ready to commit?  I'm not very
> > happy that such a large patch went from "Needs review" to "Committed" in
> > the blink of an eye on the very last commitfest day ... and artifacts like
> > this aren't doing anything to increase my confidence in it.
>
> +1.  I wonder if this should be reverted entirely.
This also broke pg_dump when connecting to pre-9.6 systems.  That's a
pretty easy fix and I was just about to push a fix for that, but will
hold off if this is going to be reverted..

Thanks!

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Peter Geoghegan-3
In reply to this post by Robert Haas
On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas <[hidden email]> wrote:
>> I assume that the problem here is larger than just failure to adhere to
>> C89 comment style.  Was this patch really ready to commit?  I'm not very
>> happy that such a large patch went from "Needs review" to "Committed" in
>> the blink of an eye on the very last commitfest day ... and artifacts like
>> this aren't doing anything to increase my confidence in it.
>
> +1.  I wonder if this should be reverted entirely.

I really wish I could have done more to help with this, but I didn't
do enough soon enough. Regrettably, I think that the patch just isn't
ready. For example, the way that expression indexes just aren't
handled is a cause for concern, as is the general way in which high
keys are modified during index builds. Interactions with logical
decoding are also a concern; there could be significant issues there,
but that analysis just didn't happen. I had significant
misunderstandings about the patch as recently as this week.

This should be reverted.

--
Peter Geoghegan


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Tom Lane-2
Peter Geoghegan <[hidden email]> writes:
> On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas <[hidden email]> wrote:
>> +1.  I wonder if this should be reverted entirely.

> I really wish I could have done more to help with this, but I didn't
> do enough soon enough. Regrettably, I think that the patch just isn't
> ready. For example, the way that expression indexes just aren't
> handled is a cause for concern, as is the general way in which high
> keys are modified during index builds. Interactions with logical
> decoding are also a concern; there could be significant issues there,
> but that analysis just didn't happen. I had significant
> misunderstandings about the patch as recently as this week.

> This should be reverted.

Given those concerns, this *clearly* was not ready to commit.
Please revert, Teodor.

                        regards, tom lane


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Teodor Sigaev
> Given those concerns, this *clearly* was not ready to commit.
> Please revert, Teodor.

Will do, sorry. I was a bit confused with quiet discussion

--
Teodor Sigaev                                   E-mail: [hidden email]
                                                    WWW: http://www.sigaev.ru/


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Anastasia Lubennikova
In reply to this post by Peter Geoghegan-3
Peter Geoghegan-3 wrote
On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas <[hidden email]> wrote:
>> I assume that the problem here is larger than just failure to adhere to
>> C89 comment style.  Was this patch really ready to commit?  I'm not very
>> happy that such a large patch went from "Needs review" to "Committed" in
>> the blink of an eye on the very last commitfest day ... and artifacts like
>> this aren't doing anything to increase my confidence in it.
It was just missed comment after the fix between patch revisions.

I really wish I could have done more to help with this, but I didn't
do enough soon enough. Regrettably, I think that the patch just isn't
ready. For example, the way that expression indexes just aren't
handled is a cause for concern, as is the general way in which high
keys are modified during index builds. Interactions with logical
decoding are also a concern; there could be significant issues there,
but that analysis just didn't happen. I had significant
misunderstandings about the patch as recently as this week.

This should be reverted.
The answer to the question about expressions is quite simple - they are not supported by index-only scan, so having them in covering index now is just wasting of disc space.

It's sad to hear that patch should be reverted. But, of course, I can't argue with community's decision.
Reply | Threaded
Open this post in threaded view
|

Re: pgsql: CREATE INDEX ... INCLUDING (column[, ...])

Tom Lane-2
Anastasia Lubennikova <[hidden email]> writes:
> The answer to the question about expressions is quite simple - they are not
> supported by index-only scan, so having them in covering index now is just
> wasting of disc space.

Well, it's true that the planner can't handle them easily in IOS, but
your claim that that makes them useless is exactly backwards.  As an
example, consider an index on x with f(x) as an extra column.  The
planner *could* make use of f(x), at least in simple cases, because
the presence of x would bypass the lack of intelligence in
check_index_only().

In any case, work is afoot to fix that planner restriction, so I do
not think we should add features that expect it to be a permanent
part of the landscape.

                        regards, tom lane


--
Sent via pgsql-committers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers