Thread: pgsql: CREATE INDEX ... INCLUDING (column[, ...])
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(-)
Teodor Sigaev <teodor@sigaev.ru> 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
On Fri, Apr 8, 2016 at 2:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Teodor Sigaev <teodor@sigaev.ru> 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
* Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Apr 8, 2016 at 2:29 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Teodor Sigaev <teodor@sigaev.ru> 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
Attachment
On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas <robertmhaas@gmail.com> 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
Peter Geoghegan <pg@heroku.com> writes:
> On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas <robertmhaas@gmail.com> 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
> 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: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Peter Geoghegan-3 wrote > On Fri, Apr 8, 2016 at 11:31 AM, Robert Haas < > robertmhaas@ > > 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. -- View this message in context: http://postgresql.nabble.com/pgsql-CREATE-INDEX-INCLUDING-column-tp5897653p5897721.html Sent from the PostgreSQL - committers mailing list archive at Nabble.com.
Anastasia Lubennikova <a.lubennikova@postgrespro.ru> 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