WIP: Covering + unique indexes.
Hi hackers,
I'm working on a patch that allows to combine covering and unique
functionality for btree indexes.
_Previous discussion was here:_
1) Proposal thread
</messages/by-id/55F2CCD0.7040608@postgrespro.ru>
2) Message with proposal clarification
</messages/by-id/55F84DF4.5030207@postgrespro.ru>
In a nutshell, the feature allows to create index with "key" columns and
"included" columns.
"key" columns can be used as scan keys. Unique constraint relates only
to "key" columns.
"included" columns may be used as scan keys if they have suitable opclass.
Both "key" and "included" columns can be returned from index by
IndexOnlyScan.
Btree is the default index and it's used everywhere. So it requires
properly testing. Volunteers are welcome)
_Use case:_
- We have a table (c1, c2, c3, c4);
- We need to have an unique index on (c1, c2).
- We would like to have a covering index on all columns to avoid reading
of heap pages.
Old way:
CREATE UNIQUE INDEX olduniqueidx ON oldt USING btree (c1, c2);
CREATE INDEX oldcoveringidx ON oldt USING btree (c1, c2, c3, c4);
What's wrong?
Two indexes contain repeated data. Overhead to data manipulation
operations and database size.
New way:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);
The patch is attached.
In 'test.sql' you can find a test with detailed comments on each step,
and comparison of old and new indexes.
New feature has following syntax:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);
Keyword INCLUDING defines the "included" columns of index. These columns
aren't concern to unique constraint.
Also, them are not stored in index inner pages. It allows to decrease
index size.
_Results:_
1) Additional covering index is not required anymore.
2) New index can use IndexOnlyScan on queries, where old index can't.
For example,
explain analyze select c1, c2 from newt where c1<10000 and c3<20;
*more examples in 'test.sql'
_Future work:_
To do opclasses for "included" columns optional.
CREATE TABLE tbl (c1 int, c4 box);
CREATE UNIQUE INDEX idx ON tbl USING btree (c1) INCLUDING (c4);
If we don't need c4 as an index scankey, we don't need any btree opclass
on it.
But we still want to have it in covering index for queries like
SELECT c4 FROM tbl WHERE c1=1000;
SELECT * FROM tbl WHERE c1=1000;
--
Anastasia Lubennikova
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
Import Notes
Reply to msg id not found: 56167F09.3090101@postgrespro.ruReference msg id not found: 56167F09.3090101@postgrespro.ru
On 8 October 2015 at 16:18, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
Hi hackers,
I'm working on a patch that allows to combine covering and unique
functionality for btree indexes.Previous discussion was here:
1) Proposal thread
2) Message with proposal clarificationIn a nutshell, the feature allows to create index with "key" columns and
"included" columns.
"key" columns can be used as scan keys. Unique constraint relates only to
"key" columns.
"included" columns may be used as scan keys if they have suitable opclass.
Both "key" and "included" columns can be returned from index by
IndexOnlyScan.Btree is the default index and it's used everywhere. So it requires properly
testing. Volunteers are welcome)Use case:
- We have a table (c1, c2, c3, c4);
- We need to have an unique index on (c1, c2).
- We would like to have a covering index on all columns to avoid reading of
heap pages.Old way:
CREATE UNIQUE INDEX olduniqueidx ON oldt USING btree (c1, c2);
CREATE INDEX oldcoveringidx ON oldt USING btree (c1, c2, c3, c4);What's wrong?
Two indexes contain repeated data. Overhead to data manipulation operations
and database size.New way:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);The patch is attached.
In 'test.sql' you can find a test with detailed comments on each step, and
comparison of old and new indexes.New feature has following syntax:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);
Keyword INCLUDING defines the "included" columns of index. These columns
aren't concern to unique constraint.
Also, them are not stored in index inner pages. It allows to decrease index
size.Results:
1) Additional covering index is not required anymore.
2) New index can use IndexOnlyScan on queries, where old index can't.For example,
explain analyze select c1, c2 from newt where c1<10000 and c3<20;*more examples in 'test.sql'
Future work:
To do opclasses for "included" columns optional.CREATE TABLE tbl (c1 int, c4 box);
CREATE UNIQUE INDEX idx ON tbl USING btree (c1) INCLUDING (c4);If we don't need c4 as an index scankey, we don't need any btree opclass on
it.
But we still want to have it in covering index for queries likeSELECT c4 FROM tbl WHERE c1=1000;
SELECT * FROM tbl WHERE c1=1000;
The definition output needs a space after "INCLUDING":
# SELECT pg_get_indexdef('people_first_name_last_name_email_idx'::regclass::oid);
pg_get_indexdef
--------------------------------------------------------------------------------------------------------------------------
CREATE UNIQUE INDEX people_first_name_last_name_email_idx ON people
USING btree (first_name, last_name) INCLUDING(email)
(1 row)
There is also no collation output:
# CREATE UNIQUE INDEX test_idx ON people (first_name COLLATE "en_GB",
last_name) INCLUDING (email COLLATE "pl_PL");
CREATE INDEX
# SELECT pg_get_indexdef('test_idx'::regclass::oid);
pg_get_indexdef
-------------------------------------------------------------------------------------------------------------
CREATE UNIQUE INDEX test_idx ON people USING btree (first_name
COLLATE "en_GB", last_name) INCLUDING(email)
(1 row)
As for functioning, it works as described:
# EXPLAIN SELECT email FROM people WHERE (first_name,last_name) =
('Paul','Freeman');
QUERY PLAN
----------------------------------------------------------------------------------------------------------
Index Only Scan using people_first_name_last_name_email_idx on people
(cost=0.28..1.40 rows=1 width=21)
Index Cond: ((first_name = 'Paul'::text) AND (last_name = 'Freeman'::text))
(2 rows)
Typo:
"included columns must not intersects with key columns"
should be:
"included columns must not intersect with key columns"
One thing I've noticed you can do with your patch, which you haven't
mentioned, is have a non-unique covering index:
# CREATE INDEX covering_idx ON people (first_name) INCLUDING (last_name);
CREATE INDEX
# EXPLAIN SELECT first_name, last_name FROM people WHERE first_name = 'Paul';
QUERY PLAN
---------------------------------------------------------------------------------
Index Only Scan using covering_idx on people (cost=0.28..1.44 rows=4 width=13)
Index Cond: (first_name = 'Paul'::text)
(2 rows)
But this appears to behave as if it were a regular multi-column index,
in that it will use the index for ordering rather than sort after
fetching from the index. So is this really stored the same as a
multi-column index? The index sizes aren't identical, so something is
different.
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
08.10.2015 19:31, Thom Brown пишет:
On 8 October 2015 at 16:18, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Hi hackers,
I'm working on a patch that allows to combine covering and unique
functionality for btree indexes.Previous discussion was here:
1) Proposal thread
2) Message with proposal clarificationIn a nutshell, the feature allows to create index with "key" columns and
"included" columns.
"key" columns can be used as scan keys. Unique constraint relates only to
"key" columns.
"included" columns may be used as scan keys if they have suitable opclass.
Both "key" and "included" columns can be returned from index by
IndexOnlyScan.Btree is the default index and it's used everywhere. So it requires properly
testing. Volunteers are welcome)Use case:
- We have a table (c1, c2, c3, c4);
- We need to have an unique index on (c1, c2).
- We would like to have a covering index on all columns to avoid reading of
heap pages.Old way:
CREATE UNIQUE INDEX olduniqueidx ON oldt USING btree (c1, c2);
CREATE INDEX oldcoveringidx ON oldt USING btree (c1, c2, c3, c4);What's wrong?
Two indexes contain repeated data. Overhead to data manipulation operations
and database size.New way:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);The patch is attached.
In 'test.sql' you can find a test with detailed comments on each step, and
comparison of old and new indexes.New feature has following syntax:
CREATE UNIQUE INDEX newidx ON newt USING btree (c1, c2) INCLUDING (c3, c4);
Keyword INCLUDING defines the "included" columns of index. These columns
aren't concern to unique constraint.
Also, them are not stored in index inner pages. It allows to decrease index
size.Results:
1) Additional covering index is not required anymore.
2) New index can use IndexOnlyScan on queries, where old index can't.For example,
explain analyze select c1, c2 from newt where c1<10000 and c3<20;*more examples in 'test.sql'
Future work:
To do opclasses for "included" columns optional.CREATE TABLE tbl (c1 int, c4 box);
CREATE UNIQUE INDEX idx ON tbl USING btree (c1) INCLUDING (c4);If we don't need c4 as an index scankey, we don't need any btree opclass on
it.
But we still want to have it in covering index for queries likeSELECT c4 FROM tbl WHERE c1=1000;
SELECT * FROM tbl WHERE c1=1000;The definition output needs a space after "INCLUDING":
# SELECT pg_get_indexdef('people_first_name_last_name_email_idx'::regclass::oid);
pg_get_indexdef
--------------------------------------------------------------------------------------------------------------------------
CREATE UNIQUE INDEX people_first_name_last_name_email_idx ON people
USING btree (first_name, last_name) INCLUDING(email)
(1 row)There is also no collation output:
# CREATE UNIQUE INDEX test_idx ON people (first_name COLLATE "en_GB",
last_name) INCLUDING (email COLLATE "pl_PL");
CREATE INDEX# SELECT pg_get_indexdef('test_idx'::regclass::oid);
pg_get_indexdef
-------------------------------------------------------------------------------------------------------------
CREATE UNIQUE INDEX test_idx ON people USING btree (first_name
COLLATE "en_GB", last_name) INCLUDING(email)
(1 row)As for functioning, it works as described:
# EXPLAIN SELECT email FROM people WHERE (first_name,last_name) =
('Paul','Freeman');
QUERY PLAN
----------------------------------------------------------------------------------------------------------
Index Only Scan using people_first_name_last_name_email_idx on people
(cost=0.28..1.40 rows=1 width=21)
Index Cond: ((first_name = 'Paul'::text) AND (last_name = 'Freeman'::text))
(2 rows)Typo:
"included columns must not intersects with key columns"
should be:
"included columns must not intersect with key columns"
Thank you for testing. Mentioned issues are fixed.
One thing I've noticed you can do with your patch, which you haven't
mentioned, is have a non-unique covering index:# CREATE INDEX covering_idx ON people (first_name) INCLUDING (last_name);
CREATE INDEX# EXPLAIN SELECT first_name, last_name FROM people WHERE first_name = 'Paul';
QUERY PLAN
---------------------------------------------------------------------------------
Index Only Scan using covering_idx on people (cost=0.28..1.44 rows=4 width=13)
Index Cond: (first_name = 'Paul'::text)
(2 rows)But this appears to behave as if it were a regular multi-column index,
in that it will use the index for ordering rather than sort after
fetching from the index. So is this really stored the same as a
multi-column index? The index sizes aren't identical, so something is
different.
Yes, it behaves as a regular multi-column index.
Index sizes are different, because included attributes are not stored in
index inner pages.
It allows to decrease index size. I don't sure that it doesn't decrease
search speed.
But I assumed that we are never execute search on included columns
without clause on key columns.
So it must be not too costly to recheck included attributes on leaf pages.
Furthermore, it's a first step of work on "optional oplasses for
included columns".
If attribute hasn't opclass, we certainly don't need to store it in
inner index page.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
covering_unique_2.0.patchtext/x-patch; name=covering_unique_2.0.patchDownload+246-72
Finally, completed patch "covering_unique_3.0.patch" is here.
It includes the functionality discussed above in the thread, regression
tests and docs update.
I think it's quite ready for review.
_Future work:_
Besides that, I'd like to get feedback about attached patch
"optional_opclass_3.0.patch".
It should be applied on the "covering_unique_3.0.patch".
Actually, this patch is the first step to do opclasses for "included"
columns optional
and implement real covering indexing.
Example:
CREATE TABLE tbl (c1 int, c4 box);
CREATE UNIQUE INDEX idx ON tbl USING btree (c1) INCLUDING (c4);
If we don't need c4 as an index scankey, we don't need any btree opclass
on it.
But we still want to have it in covering index for queries like
SELECT c4 FROM tbl WHERE c1=1000; // uses the IndexOnlyScan
SELECT * FROM tbl WHERE c1=1000; // uses the IndexOnlyScan
The patch "optional_opclass" completely ignores opclasses of included
attributes.
To see the difference, look at the explain analyze output:
explain analyze select * from tbl where c1=2 and c4 && box '(0,0,1,1)';
QUERY PLAN
---------------------------------------------------------------------------------------------------------------
Index Only Scan using idx on tbl (cost=0.13..4.15 rows=1 width=36)
(actual time=0.010..0.013 rows=1 loops=1)
Index Cond: (c1 = 2)
Filter: (c4 && '(1,1),(0,0)'::box)
"Index Cond" shows the index ScanKey conditions and "Filter" is for
conditions which are used after index scan. Anyway it is faster than
SeqScan that we had before, because IndexOnlyScan avoids extra heap fetches.
As I already said, this patch is just WIP, so included opclass is not
"optional" but actually "ignored".
And following example works worse than without the patch. Please, don't
care about it.
CREATE TABLE tbl2 (c1 int, c2 int);
CREATE UNIQUE INDEX idx2 ON tbl2 USING btree (c1) INCLUDING (c2);
explain analyze select * from tbl2 where c1<20 and c2<5;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Index Only Scan using idx2 on tbl2 (cost=0.28..4.68 rows=10 width=8)
(actual time=0.055..0.066 rows=9 loops=1)
Index Cond: (c1 < 20)
Filter: (c2 < 5)
The question is more about suitable syntax.
We have two different optimizations here:
1. INCLUDED columns
2. Optional opclasses
It's logical to provide optional opclasses only for included columns.
Is it ok, to handle it using the same syntax and resolve all opclass
conflicts while create index?
CREATE TABLE tbl2 (c1 int, c2 int, c4 box);
CREATE UNIQUE INDEX idx2 ON tbl2 USING btree (c1) INCLUDING (c2, c4);
CREATE UNIQUE INDEX idx3 ON tbl2 USING btree (c1) INCLUDING (c4, c2);
Of course, order of attributes is important.
Attrs which have oplass and want to use it in ScanKey must be situated
before the others.
idx2 will use c2 in IndexCond, while idx3 will not. But I think that
it's the job for DBA.
If you see any related changes in planner, please mention them. I
haven't explored that part of code yet and could have missed something.
--
Anastasia Lubennikova
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
On Tue, Dec 1, 2015 at 7:53 AM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
If we don't need c4 as an index scankey, we don't need any btree opclass on
it.
But we still want to have it in covering index for queries likeSELECT c4 FROM tbl WHERE c1=1000; // uses the IndexOnlyScan
SELECT * FROM tbl WHERE c1=1000; // uses the IndexOnlyScanThe patch "optional_opclass" completely ignores opclasses of included
attributes.
OK, I don't get it. Why have an opclass here at all, even optionally?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
03.12.2015 04:03, Robert Haas пишет:
On Tue, Dec 1, 2015 at 7:53 AM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:If we don't need c4 as an index scankey, we don't need any btree opclass on
it.
But we still want to have it in covering index for queries likeSELECT c4 FROM tbl WHERE c1=1000; // uses the IndexOnlyScan
SELECT * FROM tbl WHERE c1=1000; // uses the IndexOnlyScanThe patch "optional_opclass" completely ignores opclasses of included
attributes.OK, I don't get it. Why have an opclass here at all, even optionally?
We haven't opclass on c4 and there's no need to have it.
But now (without a patch) it's impossible to create covering index,
which contains columns with no opclass for btree.
test=# create index on tbl using btree (c1, c4);
ERROR: data type box has no default operator class for access method
"btree"
ComputeIndexAttrs() processes the list of index attributes and trying to
get an opclass for each of them via GetIndexOpClass().
The patch drops this check for included attributes. So it makes possible
to store any datatype in btree and use IndexOnlyScan advantages.
I hope that this helps to clarify.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Dec 1, 2015 at 4:53 AM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
Finally, completed patch "covering_unique_3.0.patch" is here.
It includes the functionality discussed above in the thread, regression
tests and docs update.
I think it's quite ready for review.
Thanks for the patch.
I get a compiler warning when building it on gcc (SUSE Linux) 4.8.1
20130909 [gcc-4_8-branch revision 202388]:
nbtinsert.c: In function '_bt_check_unique':
nbtinsert.c:256:2: warning: ISO C90 forbids mixed declarations and
code [-Wdeclaration-after-statement]
SnapshotData SnapshotDirty;
^
And the dblink contrib module fails its make check.
I'm trying to find a good test case for it. Unfortunately in most of
my natural use cases, the inclusion of the extra column causes the
updates to become non-HOT, which causes more problems than it solves.
Cheers,
Jeff
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Dec 26, 2015 at 5:58 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
And the dblink contrib module fails its make check.
Ignore the dblink complaint. It seems to have been some wonky build
issue that is not reproducible.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2 December 2015 at 01:53, Anastasia Lubennikova <
a.lubennikova@postgrespro.ru> wrote:
Finally, completed patch "covering_unique_3.0.patch" is here.
It includes the functionality discussed above in the thread, regression
tests and docs update.
I think it's quite ready for review.
Hi Anastasia,
I've maybe mentioned before that I think this is a great feature and I
think it will be very useful to have, so I've signed up to review the
patch, and below is the results of my first pass from reading the code.
Apologies if some of the things seem like nitpicks, I've basically just
listed everything I've noticed during, no matter how small.
- int natts = rel->rd_rel->relnatts;
+ int nkeyatts = rel->rd_index->indnkeyatts;
+
+ Assert (rel->rd_index != NULL);
+ Assert(rel->rd_index->indnatts != 0);
+ Assert(rel->rd_index->indnkeyatts != 0);
+
SnapshotData SnapshotDirty;There's a couple of problems here. According to [1]http://www.postgresql.org/docs/devel/static/source-conventions.html#AEN111267 the C code must follow
the C89 standard, but this appears not to. You have some statements before
the final variable declaration, and also there's a problem as you're
Asserting that rel->rd_index != NULL after already trying to dereference it
in the assignment to nkeyatts, which makes the Assert() useless.
+ An access method that supports this feature sets
<structname>pg_am</>.<structfield>amcanincluding</> true.
I don't think this belongs under the "Index Uniqueness Checks" title. I
think the "Columns included with clause INCLUDING aren't used to enforce
uniqueness." that you've added before it is a good idea, but perhaps the
details of amcanincluding are best explained elsewhere.
- indexed columns are equal in multiple rows.
+ indexed columns are equal in multiple rows. Columns included with clause
+ INCLUDING aren't used to enforce constraints (UNIQUE, PRIMARY KEY,
etc).<literal> is missing around "INCLUDING" here. Perhaps this part needs more
explanation in a new paragraph. Likely it's good idea to also inform the
reader that the columns which are part of the INCLUDING clause exist only
to allow the query planner to skip having to perform a lookup to the heap
when all of the columns required for the relation are present in the
indexed columns, or in the INCLUDING columns. I think you should explain
that the index can also only be used as pre-sorted input for columns which
are in the "indexed columns" part of the index, and the INCLUDING column
are not searchable as index quals.
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -137,7 +137,6 @@ CheckIndexCompatible(Oid oldId,
Relation irel;
int i;
Datum d;
-
/* Caller should already have the relation locked in some way. */You've accidentally removed an empty line here.
+ /*
+ * All information about key and included cols is in numberOfKeyAttributes
number.
+ * So we can concat all index params into one list.
+ */
+ stmt->indexParams = list_concat(stmt->indexParams,
stmt->indexIncludingParams);I think this should be explained with a better comment, perhaps:
/*
* We append any INCLUDING columns onto the indexParams list so that
* we have one list with all columns. Later we can determine which of these
* are indexed, and which are just part of the INCLUDING list by check the
list
* position. A list item in a position less than ii_NumIndexKeyAttrs is
part of
* the indexed columns, and anything equal to and over is part of the
* INCLUDING columns.
*/
+ stack = _bt_search(rel, IndexRelationGetNumberOfKeyAttributes(rel),
itup_scankey,
This line is longer than 80 chars.
+ /* Truncate nonkey attributes when inserting on nonleaf pages */
+ if (wstate->index->rd_index->indnatts !=
wstate->index->rd_index->indnkeyatts)
+ {
+ BTPageOpaque pageop = (BTPageOpaque) PageGetSpecialPointer(npage);
+
+ if (!P_ISLEAF(pageop))
+ {
+ itup = index_reform_tuple(wstate->index, itup,
wstate->index->rd_index->indnatts, wstate->index->rd_index->indnkeyatts);
+ itupsz = IndexTupleDSize(*itup);
+ itupsz = MAXALIGN(itupsz);
+ }
+ }A few of the lines here are over 80 chars.
+ This clause specifies additional columns to be appended to the set
of index columns.
+ Included columns don't support any constraints <literal>(UNIQUE,
PRMARY KEY, EXCLUSION CONSTRAINT)</>.
+ These columns can improve the performance of some queries through
using advantages of index-only scan
+ (Or so called <firstterm>covering</firstterm> indexes. Covering
index is the index that
+ covers all columns required in the query and prevents a table
access).
+ Besides that, included attributes are not stored in index inner
pages.
+ It allows to decrease index size and furthermore it provides a way
to extend included
+ columns to store atttributes without suitable opclass (not
implemented yet).
+ This clause could be applied to both unique and nonunique indexes.
+ It's possible to have non-unique covering index, which behaves as
a regular
+ multi-column index with a bit smaller index-size.
+ Currently, only the B-tree access method supports this feature."PRMARY KEY" should be "PRIMARY KEY". I ended up rewriting this paragraph
as follows.
"An optional <literal>INCLUDING</> clause allows a list of columns to be
specified which will be included in the index, in the non-key portion of
the index. Columns which are part of this clause cannot also exist in the
indexed columns portion of the index, and vice versa. The
<literal>INCLUDING</> columns exist solely to allow more queries to benefit
from <firstterm>index only scans</> by including certain columns in the
index, the value of which would otherwise have to be obtained by reading
the table's heap. Having these columns in the <literal>INCLUDING</> clause
in some cases allows <productname>PostgreSQL</> to skip the heap read
completely. This also allows <literal>UNIQUE</> indexes to be defined on
one set of columns, which can include another set of column in the
<literal>INCLUDING</> clause, on which the uniqueness is not enforced upon.
This can also be useful for non-unique indexes as any columns which are not
required for the searching or ordering of records can defined in the
<literal>INCLUDING</> clause, which can often reduce the size of the index."
Maybe not perfect, but maybe it's an improvement?
+ To create an unique B-tree index on the column <literal>title</literal>
in
and
+ To create an unique B-tree index on the column <literal>title</literal>
Although "unique" starts with a vowel, "an" is not correct here: This is
best explained in someone else's words:
"The choice between a and an is governed not by whether the next written
letter is a consonant or vowel but by whether the next word begins with the
sound of a vowel or consonant. Unique begins with a "y" sound, hence a
unique is correct."
- int natts = rel->rd_rel->relnatts;
+ int nkeyatts = rel->rd_rel->relnatts;
ScanKey itup_scankey;
BTStack stack;
Buffer buf;
OffsetNumber offset;
+ Assert (rel->rd_index != NULL);
+ Assert(rel->rd_index->indnatts != 0);
+ Assert(rel->rd_index->indnkeyatts != 0);
+ nkeyatts = IndexRelationGetNumberOfKeyAttributes(rel);
+nkeyatts is assigned twice.
+ /* Truncate nonkey attributes when inserting on nonleaf pages. */
+ if (rel->rd_index->indnatts != rel->rd_index->indnkeyatts)
+ if (!P_ISLEAF(lpageop))
+ itup = index_reform_tuple(rel, itup, rel->rd_index->indnatts,
rel->rd_index->indnkeyatts);I don't recall having seen any places in the code which skip on the outer
{} braces in this way before, although I can't see anything in the coding
standards which states that this is wrong. In either case, perhaps it's
better to just use an && instead of the extra if (). The assignment line
also exceeds 80 chars.
+/*
+ * Reform index tuple. Truncate nonkey (INCLUDED) attributes.
+ */I guess "INCLUDED" should be "INCLUDING"? The capitalisation makes me think
you're talking about the syntax.
+ if (!colno || colno == keyno + 1) {
appendStringInfoString(&buf, quote_identifier(attname));
+ if ((attrsOnly)&&(keyno >= idxrec->indnkeyatts))
+ appendStringInfoString(&buf, " (included)");
+ }The { brace here should be on the next line. I'm also a bit unsure what the
"(included)" is for. There's also surplus parenthesis in the 2nd "if"
statement, and also missing whitespace.
+ bool amcanincluding; /* does AM support INCLUDING columns? */
Perhaps this should be called "amcaninclude". I don't think we really need
to use the same word as is used in the SQL syntax here, do we?
Same for the new column in pg_am.
Perhaps this needs the comment updated from the standard one.
int16 indnatts; /* number of columns in index */
maybe just say /* total number of columns in index */ ?
+ int ii_NumIndexKeyAttrs;
The struct comment needs an entry for ii_NumIndexKeyAttrs.
+ List *indexIncludingParams; /* additional columns to index: a list of
IndexElem */
This should wrap at 80 chars. struct RestrictInfo has some examples of how
this is normally done.
/*
+ * RelationGetNumberOfAttributes
+ * Returns the number of attributes in a relation.
+ */
+#define IndexRelationGetNumberOfKeyAttributes(relation)
((relation)->rd_index->indnkeyatts)
+Copy paste problem. You missed editing the comment.
I've not tested the patch yet. I will send another email soon with the
results of that.
Thanks for working on this.
[1]: http://www.postgresql.org/docs/devel/static/source-conventions.html#AEN111267
http://www.postgresql.org/docs/devel/static/source-conventions.html#AEN111267
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 4 January 2016 at 21:49, David Rowley <david.rowley@2ndquadrant.com>
wrote:
I've not tested the patch yet. I will send another email soon with the
results of that.
Hi,
As promised I've done some testing on this, and I've found something which
is not quite right:
create table ab (a int,b int);
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
create index on ab (a) including (b);
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)
This is what I'd expect
truncate table ab;
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
explain select * from ab order by a,b;
QUERY PLAN
------------------------------------------------------------------------------
Index Only Scan using ab_a_b_idx on ab (cost=0.15..66.87 rows=2260
width=8)
(1 row)
This index, as we've defined it should not be able to satisfy the query's
order by, although it does give correct results, that's because the index
seems to be built wrongly in cases where the rows are added after the index
exists.
If we then do:
reindex table ab;
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)
It looks normal again.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Jan 5, 2016 at 11:55 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
On 4 January 2016 at 21:49, David Rowley <david.rowley@2ndquadrant.com>
wrote:I've not tested the patch yet. I will send another email soon with the
results of that.Hi,
As promised I've done some testing on this, and I've found something which
is not quite right:create table ab (a int,b int);
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
create index on ab (a) including (b);
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)
If you set enable_sort=off, then you get the index-only scan with no
sort. So it believes the index can be used for ordering (correctly, I
think), just sometimes it thinks it is not faster to do it that way.
I'm not sure why this would be a correctness problem. The covered
column does not participate in uniqueness checks, but it still usually
participates in index ordering. (That is why dummy op-classes are
needed if you want to include non-sortable-type columns as being
covered.)
This is what I'd expect
truncate table ab;
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
explain select * from ab order by a,b;
QUERY PLAN
------------------------------------------------------------------------------
Index Only Scan using ab_a_b_idx on ab (cost=0.15..66.87 rows=2260
width=8)
(1 row)This index, as we've defined it should not be able to satisfy the query's
order by, although it does give correct results, that's because the index
seems to be built wrongly in cases where the rows are added after the index
exists.
I think this just causes differences in planner statistics leading to
different plans. ANALYZE the table and it goes back to doing the
sort, because it thinks the sort is faster.
Cheers,
Jeff
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 7 January 2016 at 06:36, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jan 5, 2016 at 11:55 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:create table ab (a int,b int);
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
create index on ab (a) including (b);
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)If you set enable_sort=off, then you get the index-only scan with no
sort. So it believes the index can be used for ordering (correctly, I
think), just sometimes it thinks it is not faster to do it that way.I'm not sure why this would be a correctness problem. The covered
column does not participate in uniqueness checks, but it still usually
participates in index ordering. (That is why dummy op-classes are
needed if you want to include non-sortable-type columns as being
covered.)
If that's the case, then it appears that I've misunderstood INCLUDING. From
reading _bt_doinsert() it appeared that it'll ignore the INCLUDING columns
and just find the insert position based on the key columns. Yet that's not
the way that it appears to work. I was also a bit confused, as from working
with another database which has very similar syntax to this, that one only
includes the columns to allow index only scans, and the included columns
are not indexed, therefore can't be part of index quals and the index only
provides a sorted path for the indexed columns, and not the included
columns.
Saying that, I'm now a bit confused to why the following does not produce 2
indexes which are the same size:
create table t1 (a int, b text);
insert into t1 select x,md5(random()::text) from generate_series(1,1000000)
x(x);
create index t1_a_inc_b_idx on t1 (a) including (b);
create index t1_a_b_idx on t1 (a,b);
select pg_relation_Size('t1_a_b_idx'),pg_relation_size('t1_a_inc_b_idx');
pg_relation_size | pg_relation_size
------------------+------------------
59064320 | 58744832
(1 row)
Also, if we want INCLUDING() to mean "uniqueness is not enforced on these
columns, but they're still in the index", then I don't really think
allowing types without a btree opclass is a good idea. It's likely too
surprised filled and might not be what the user actually wants. I'd suggest
that these non-indexed columns would be better defined by further expanding
the syntax, the first (perhaps not very good) thing that comes to mind is:
create unique index idx_name on table (unique_col) also index
(other,idx,cols) including (leaf,onlycols);
Looking up thread, I don't think I was the first to be confused by this.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
04.01.2016 11:49, David Rowley:
On 2 December 2015 at 01:53, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru <mailto:a.lubennikova@postgrespro.ru>>
wrote:Finally, completed patch "covering_unique_3.0.patch" is here.
It includes the functionality discussed above in the thread,
regression tests and docs update.
I think it's quite ready for review.Hi Anastasia,
I've maybe mentioned before that I think this is a great feature and I
think it will be very useful to have, so I've signed up to review the
patch, and below is the results of my first pass from reading the
code. Apologies if some of the things seem like nitpicks, I've
basically just listed everything I've noticed during, no matter how small.
First of all, I would like to thank you for writing such a detailed review.
All mentioned style problems, comments and typos are fixed in the patch
v4.0.
+ An access method that supports this feature sets
<structname>pg_am</>.<structfield>amcanincluding</> true.I don't think this belongs under the "Index Uniqueness Checks" title.
I think the "Columns included with clause INCLUDING aren't used to
enforce uniqueness." that you've added before it is a good idea, but
perhaps the details of amcanincluding are best explained elsewhere.
agree
+ This clause specifies additional columns to be appended to the set of index columns. + Included columns don't support any constraints <literal>(UNIQUE, PRMARY KEY, EXCLUSION CONSTRAINT)</>. + These columns can improve the performance of some queries through using advantages of index-only scan + (Or so called <firstterm>covering</firstterm> indexes. Covering index is the index that + covers all columns required in the query and prevents a table access). + Besides that, included attributes are not stored in index inner pages. + It allows to decrease index size and furthermore it provides a way to extend included + columns to store atttributes without suitable opclass (not implemented yet). + This clause could be applied to both unique and nonunique indexes. + It's possible to have non-unique covering index, which behaves as a regular + multi-column index with a bit smaller index-size. + Currently, only the B-tree access method supports this feature."PRMARY KEY" should be "PRIMARY KEY". I ended up rewriting this
paragraph as follows."An optional <literal>INCLUDING</> clause allows a list of columns to
be specified which will be included in the index, in the non-key
portion of the index. Columns which are part of this clause cannot
also exist in the indexed columns portion of the index, and vice
versa. The <literal>INCLUDING</> columns exist solely to allow more
queries to benefit from <firstterm>index only scans</> by including
certain columns in the index, the value of which would otherwise have
to be obtained by reading the table's heap. Having these columns in
the <literal>INCLUDING</> clause in some cases allows
<productname>PostgreSQL</> to skip the heap read completely. This also
allows <literal>UNIQUE</> indexes to be defined on one set of columns,
which can include another set of column in the <literal>INCLUDING</>
clause, on which the uniqueness is not enforced upon. This can also be
useful for non-unique indexes as any columns which are not required
for the searching or ordering of records can defined in the
<literal>INCLUDING</> clause, which can often reduce the size of the
index."Maybe not perfect, but maybe it's an improvement?
Yes, this explanation is much better. I've just added couple of notes.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
covering_unique_4.0.patchtext/x-patch; name=covering_unique_4.0.patchDownload+349-79
08.01.2016 00:12, David Rowley:
On 7 January 2016 at 06:36, Jeff Janes <jeff.janes@gmail.com
<mailto:jeff.janes@gmail.com>> wrote:On Tue, Jan 5, 2016 at 11:55 PM, David Rowley
<david.rowley@2ndquadrant.com
<mailto:david.rowley@2ndquadrant.com>> wrote:create table ab (a int,b int);
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
create index on ab (a) including (b);
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)If you set enable_sort=off, then you get the index-only scan with no
sort. So it believes the index can be used for ordering (correctly, I
think), just sometimes it thinks it is not faster to do it that way.I'm not sure why this would be a correctness problem. The covered
column does not participate in uniqueness checks, but it still usually
participates in index ordering. (That is why dummy op-classes are
needed if you want to include non-sortable-type columns as being
covered.)If that's the case, then it appears that I've misunderstood INCLUDING.
From reading _bt_doinsert() it appeared that it'll ignore the
INCLUDING columns and just find the insert position based on the key
columns. Yet that's not the way that it appears to work. I was also a
bit confused, as from working with another database which has very
similar syntax to this, that one only includes the columns to allow
index only scans, and the included columns are not indexed, therefore
can't be part of index quals and the index only provides a sorted path
for the indexed columns, and not the included columns.
Thank you for properly testing. Order by clause in this case definitely
doesn't work as expected.
The problem is fixed by patching a planner function
"build_index_pathkeys()'. It disables using of index if sorting of
included columns is required.
Test example works correctly now - it always performs seq scan and sort.
Saying that, I'm now a bit confused to why the following does not
produce 2 indexes which are the same size:create table t1 (a int, b text);
insert into t1 select x,md5(random()::text) from
generate_series(1,1000000) x(x);
create index t1_a_inc_b_idx on t1 (a) including (b);
create index t1_a_b_idx on t1 (a,b);
select pg_relation_Size('t1_a_b_idx'),pg_relation_size('t1_a_inc_b_idx');
pg_relation_size | pg_relation_size
------------------+------------------
59064320 | 58744832
(1 row)
I suppose you've already found that in discussion above. Included
columns are stored only in leaf index pages. The difference is the size
of attributes 'b' which are situatedin inner pages of index "t1_a_b_idx".
Also, if we want INCLUDING() to mean "uniqueness is not enforced on
these columns, but they're still in the index", then I don't really
think allowing types without a btree opclass is a good idea. It's
likely too surprised filled and might not be what the user actually
wants. I'd suggest that these non-indexed columns would be better
defined by further expanding the syntax, the first (perhaps not very
good) thing that comes to mind is:create unique index idx_name on table (unique_col) also index
(other,idx,cols) including (leaf,onlycols);Looking up thread, I don't think I was the first to be confused by this.
Included columns are still in the index physically - they are stored in
the index relation. But they are not indexedin the true sense of the
word. It's impossible to use them for index scan or ordering. At the
beginning, I've got an idea that included columns are supposed to be
used for combination of unique index on one columns and covering on
others. In a very rare instances one could prefer a non-unique index
with included columns "t1_a_inc_b_idx"to a regular multicolumn index
"t1_a_b_idx". Frankly, I didn't see such use cases at all. Index size
reduction is not considerable, while we lose some useful index
functionality on included column. I think that it should be mentioned as
a note in documentation, but I need help to phrase it clear.
But now I see the reason to create non-unique index with included
columns - lack of suitable opclass on column "b".
It's impossible to add it into the index as a key column, but that's not
a problem with INCLUDING clause.
Look at example.
create table t1 (a int, b box);
create index t1_a_inc_b_idx on t1 (a) including (b);
create index on tbl (a,b);
ERROR: data type box has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a
default operator class for the data type.
create index on tbl (a) including (b);
CREATE INDEX
This functionality is provided by the attached patch "omit_opclass_4.0",
which must be applied over covering_unique_4.0.patch.
I see what you were confused about, I'd had the same question at the
very beginning of the discussion of this patch.
Now it seems a bit more clear to me. INCLUDING columns are not used for
the searching or ordering of records, so there is no need to check
whether they have an opclass. INCLUDING columns perform as expected and
it agrees with other database experience. And this patch is completed.
But it isn't perfect definitely... I found test case to explain that.
See below.
That's why we need optional_opclass functionality, which will use
opclass where possible and omit it in other cases.
This idea have been already described in a message Re: [PROPOSAL]
Covering + unique indexes
</messages/by-id/55F84DF4.5030207@postgrespro.ru>as
"partially unique index".
I suggest to separate optional_opclass task to ease syntax discussion
and following review. And I'll implement it in the next patch a bit later.
Test case:
1) patch covering_unique_4.0 + test_covering_unique_4.0
If included columns' opclasses are used, new query plan is the same with
the old one.
and have nearly the same execution time:
QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------
Index Only Scan using oldcoveringidx on oldt (cost=0.43..301.72
rows=1 width=8) (actual time=0.021..0.676 rows=6 loops=1)
Index Cond: ((c1 < 10000) AND (c3 < 20))
Heap Fetches: 0
Planning time: 0.101 ms
Execution time: 0.697 ms
(5 rows)
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
Index Only Scan using newidx on newt (cost=0.43..276.51 rows=1
width=8) (actual time=0.020..0.665 rows=6 loops=1)
Index Cond: ((c1 < 10000) AND (c3 < 20))
Heap Fetches: 0
Planning time: 0.082 ms
Execution time: 0.687 ms
(5 rows)
2) patch covering_unique_4.0 + patch omit_opclass_4.0 +
test_covering_unique_4.0
Otherwise, new query can not use included column in Index Cond and uses
filter instead. It slows down the query significantly.
QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------
Index Only Scan using oldcoveringidx on oldt (cost=0.43..230.39
rows=1 width=8) (actual time=0.021..0.722 rows=6 loops=1)
Index Cond: ((c1 < 10000) AND (c3 < 20))
Heap Fetches: 0
Planning time: 0.091 ms
Execution time: 0.744 ms
(5 rows)
QUERY PLAN
--------------------------------------------------------------------------------------------------------------------
Index Only Scan using newidx on newt (cost=0.43..374.68 rows=1
width=8) (actual time=0.018..2.595 rows=6 loops=1)
Index Cond: (c1 < 10000)
Filter: (c3 < 20)
Rows Removed by Filter: 9993
Heap Fetches: 0
Planning time: 0.078 ms
Execution time: 2.612 ms
--
Anastasia Lubennikova
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
On Tue, Jan 12, 2016 at 8:59 AM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:
08.01.2016 00:12, David Rowley:
On 7 January 2016 at 06:36, Jeff Janes <jeff.janes@gmail.com> wrote:
But now I see the reason to create non-unique index with included columns -
lack of suitable opclass on column "b".
It's impossible to add it into the index as a key column, but that's not a
problem with INCLUDING clause.
Look at example.create table t1 (a int, b box);
create index t1_a_inc_b_idx on t1 (a) including (b);
create index on tbl (a,b);
ERROR: data type box has no default operator class for access method
"btree"
HINT: You must specify an operator class for the index or define a default
operator class for the data type.
create index on tbl (a) including (b);
CREATE INDEXThis functionality is provided by the attached patch "omit_opclass_4.0",
which must be applied over covering_unique_4.0.patch.
Thanks for the updates.
Why is omit_opclass a separate patch? If the included columns now
never participate in the index ordering, shouldn't it be an inherent
property of the main patch that you can "cover" things without btree
opclasses?
Are you keeping them separate just to make review easier? Or do you
think there might be a reason to commit one but not the other? I
think that if we decide not to use the omit_opclass patch, then we
should also not allow covering columns to be specified on non-unique
indexes.
It looks like the "covering" patch, with or without the "omit_opclass"
patch, does not support expressions as included columns:
create table foobar (x text, y xml);
create index on foobar (x) including (md5(x));
ERROR: unrecognized node type: 904
create index on foobar (x) including ((y::text));
ERROR: unrecognized node type: 911
I think we would probably want it to work with those (or at least to
throw a better error message).
Thanks,
Jeff
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13 January 2016 at 05:59, Anastasia Lubennikova <
a.lubennikova@postgrespro.ru> wrote:
08.01.2016 00:12, David Rowley:
On 7 January 2016 at 06:36, Jeff Janes <jeff.janes@gmail.com> wrote:
On Tue, Jan 5, 2016 at 11:55 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:create table ab (a int,b int);
insert into ab select x,y from generate_series(1,20) x(x),
generate_series(10,1,-1) y(y);
create index on ab (a) including (b);
explain select * from ab order by a,b;
QUERY PLAN
----------------------------------------------------------
Sort (cost=10.64..11.14 rows=200 width=8)
Sort Key: a, b
-> Seq Scan on ab (cost=0.00..3.00 rows=200 width=8)
(3 rows)If you set enable_sort=off, then you get the index-only scan with no
sort. So it believes the index can be used for ordering (correctly, I
think), just sometimes it thinks it is not faster to do it that way.I'm not sure why this would be a correctness problem. The covered
column does not participate in uniqueness checks, but it still usually
participates in index ordering. (That is why dummy op-classes are
needed if you want to include non-sortable-type columns as being
covered.)If that's the case, then it appears that I've misunderstood INCLUDING.
From reading _bt_doinsert() it appeared that it'll ignore the INCLUDING
columns and just find the insert position based on the key columns. Yet
that's not the way that it appears to work. I was also a bit confused, as
from working with another database which has very similar syntax to this,
that one only includes the columns to allow index only scans, and the
included columns are not indexed, therefore can't be part of index quals
and the index only provides a sorted path for the indexed columns, and not
the included columns.Thank you for properly testing. Order by clause in this case definitely
doesn't work as expected.
The problem is fixed by patching a planner function
"build_index_pathkeys()'. It disables using of index if sorting of included
columns is required.
Test example works correctly now - it always performs seq scan and sort.
Thank you for updating the patch.
That's cleared up my confusion. All the code I read seemed to indicate that
INCLUDING columns were leaf only, it just confused me as to why the indexed
appeared to search and order on all columns, including the including
columns. Thanks for clearing up my confusion and fixing the patch.
Saying that, I'm now a bit confused to why the following does not produce
2 indexes which are the same size:create table t1 (a int, b text);
insert into t1 select x,md5(random()::text) from
generate_series(1,1000000) x(x);
create index t1_a_inc_b_idx on t1 (a) including (b);
create index t1_a_b_idx on t1 (a,b);
select pg_relation_Size('t1_a_b_idx'),pg_relation_size('t1_a_inc_b_idx');
pg_relation_size | pg_relation_size
------------------+------------------
59064320 | 58744832
(1 row)I suppose you've already found that in discussion above. Included columns
are stored only in leaf index pages. The difference is the size of
attributes 'b' which are situated in inner pages of index "t1_a_b_idx".
Yeah, I saw that from the code too. I just was confused as they appeared to
work like normal indexes.
I've made another pass of the covering_unique_4.0.patch. Again somethings
are nit picky (sorry), but it made sense to write them down as I noticed
them.
- multiple entries with identical keys. An access method that supports
this
+ multiple entries with identical keys. An access method that supports
thisSpace removed by mistake.
feature sets <structname>pg_am</>.<structfield>amcanunique</> true.
- (At present, only b-tree supports it.)
+ Columns included with clause INCLUDING aren't used to enforce
uniqueness.
+ (At present, only b-tree supports them.)Maybe
+ (At present <structfield>amcanunique</> is only supported by b-tree
+ indexes.)As the extra line you've added confuses what "it" or "them" means, so maybe
best to clarify that.
+ <literal>INCLUDING</literal> aren't used to enforce constraints
(UNIQUE, PRIMARY KEY, etc).
Goes beyond 80 chars.
right_item = CopyIndexTuple(item);
+ right_item = index_reform_tuple(rel, right_item, rel->rd_index->indnatts,
rel->rd_index->indnkeyatts);
Duplicate assignment. Should this perhaps be:
+ if (rel->rd_index->indnatts == rel->rd_index->indnkeyatts)
+ right_item = CopyIndexTuple(item);
+ else
+ right_item = index_reform_tuple(rel, right_item, rel->rd_index->indnatts,
rel->rd_index->indnkeyatts);?
- natts = RelationGetNumberOfAttributes(rel);
- indoption = rel->rd_indoption;
- skey = (ScanKey) palloc(natts * sizeof(ScanKeyData));
+ Assert(rel->rd_index->indnkeyatts != 0);
+ Assert(rel->rd_index->indnkeyatts <= rel->rd_index->indnatts);- for (i = 0; i < natts; i++)
+ nkeyatts = rel->rd_index->indnkeyatts;Since RelationGetNumberOfAttributes() was previously used, maybe you should
do:
+ nkeyatts = IndexRelationGetNumberOfKeyAttributes(rel);
?
Yet I'm not really sure if there is some rule about when
RelationGetNumberOfAttributes(rel) is used and when rel->->rd_rel->relnatts
is used. It seems so mixed up.
accessMethodName = stmt->accessMethod;
+
tuple = SearchSysCache1(AMNAME, PointerGetDatum(accessMethodName));
Unrelated change.
+#define Anum_pg_am_amcaninclude 15
Needs 1 more tab so that "15" lines up with the other numbers.
typedef struct IndexInfo
{
NodeTag type;
- int ii_NumIndexAttrs;
+ int ii_NumIndexAttrs; /* total number of columns in index */
+ int ii_NumIndexKeyAttrs; /* number of key columns in index */The comment above this struct still needs a comment for "NumIndexKeyAttrs".
I'm not sure exactly why there's comments in both places with that struct,
but it makes sense to follow what's been done already.
+ * Returns the number of key attributes in a relation.
I think "relation" should be "index".
Here's a few things that I'm not too sure on, which maybe Jeff or others
could give their opinion on:
ERROR: duplicate key value violates unique constraint
"covering_index_index"
DETAIL: Key (f1, f2, f3 (included))=(1, 2, BBB) already exists.
Should we only display the key columns here? f3 feels like it does not
belong in any reports about unique violations.
+ if(list_intersection(stmt->indexParams, stmt->indexIncludingParams) !=
NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("included columns must not intersect with key columns")));
+I wonder if a bit more effort should be spent here to generate a better
message. We do a bit more in cases like:
# create table a (a int, b int, c int, a int, b int);
ERROR: column "a" specified more than once
Perhaps it would be a good idea to also report the first matching intersect
item found. Any thoughts?
# create index on ab using hash (a) including (b);
WARNING: hash indexes are not WAL-logged and their use is discouraged
ERROR: access method "hash" does not support multicolumn indexes
I wonder if it's better to report: errmsg("access method \"%s\" does not
support included columns") before the multicolumn check? It probably does
not mater that much, but if a user thought (a) including (b) was a single
column index on "a", then it's a bit confusing.
I've also done some testing:
create table ab (a int, b int);
insert into ab select a,b from generate_Series(1,10) a(a),
generate_series(1,10000) b(b);
set enable_bitmapscan=off;
set enable_indexscan=off;
select * from ab where a = 1 and b=1;
a | b
---+---
1 | 1
(1 row)
set enable_indexscan = on;
select * from ab where a = 1 and b=1;
a | b
---+---
(0 rows)
This is broken. I've not looked into why yet, but from looking at the
EXPLAIN output I was a bit surprised to see b=1 as an index condition. I'd
have expected a Filter maybe, but I've not looked at the EXPLAIN code to
see how those are determined yet.
I've not looked at the other patch yet.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 13 January 2016 at 06:47, Jeff Janes <jeff.janes@gmail.com> wrote:
Why is omit_opclass a separate patch? If the included columns now
never participate in the index ordering, shouldn't it be an inherent
property of the main patch that you can "cover" things without btree
opclasses?
I also wondered this. We can't have covering indexes without fixing the
problem with the following arrays:
info->indexkeys = (int *) palloc(sizeof(int) * ncolumns);
info->indexcollations = (Oid *) palloc(sizeof(Oid) * ncolumns);
info->opfamily = (Oid *) palloc(sizeof(Oid) * ncolumns);
These need to be sized according to the number of key columns, not the
total number of columns. Of course, the TODO item in the patch states this
too.
I don't personally think the covering_unique_4.0.patch is that close to
being too big to review, I think things would make more sense of the
omit_opclass_4.0.patch was included together with this.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
13.01.2016 04:47, David Rowley :
On 13 January 2016 at 06:47, Jeff Janes <jeff.janes@gmail.com
<mailto:jeff.janes@gmail.com>> wrote:Why is omit_opclass a separate patch? If the included columns now
never participate in the index ordering, shouldn't it be an inherent
property of the main patch that you can "cover" things without btree
opclasses?I don't personally think the covering_unique_4.0.patch is that close
to being too big to review, I think things would make more sense of
the omit_opclass_4.0.patch was included together with this.
I agree that these patches should be merged. It'll be fixed it the next
updates.
I kept them separate only for historical reasons, it was more convenient
for me to debug them. Furthermore, I wanted to show some performance
degradation caused by "omit_opclass" and give a way to reproduce it
performing test with and whithot the patch.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
13.01.2016 04:27, David Rowley:
I've also done some testing:
create table ab (a int, b int);
insert into ab select a,b from generate_Series(1,10) a(a),
generate_series(1,10000) b(b);
set enable_bitmapscan=off;
set enable_indexscan=off;select * from ab where a = 1 and b=1;
a | b
---+---
1 | 1
(1 row)set enable_indexscan = on;
select * from ab where a = 1 and b=1;
a | b
---+---
(0 rows)This is broken. I've not looked into why yet, but from looking at the
EXPLAIN output I was a bit surprised to see b=1 as an index condition.
I'd have expected a Filter maybe, but I've not looked at the EXPLAIN
code to see how those are determined yet.
Hmm... Do you use both patches?
And could you provide index definition, I can't reproduce the problem
assuming that index is created by the statement
CREATE INDEX idx ON ab (a) INCLUDING (b);
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 14 January 2016 at 02:58, Anastasia Lubennikova <
a.lubennikova@postgrespro.ru> wrote:
13.01.2016 04:27, David Rowley:
I've also done some testing:
create table ab (a int, b int);
insert into ab select a,b from generate_Series(1,10) a(a),
generate_series(1,10000) b(b);
set enable_bitmapscan=off;
set enable_indexscan=off;select * from ab where a = 1 and b=1;
a | b
---+---
1 | 1
(1 row)set enable_indexscan = on;
select * from ab where a = 1 and b=1;
a | b
---+---
(0 rows)This is broken. I've not looked into why yet, but from looking at the
EXPLAIN output I was a bit surprised to see b=1 as an index condition. I'd
have expected a Filter maybe, but I've not looked at the EXPLAIN code to
see how those are determined yet.Hmm... Do you use both patches?
And could you provide index definition, I can't reproduce the problem
assuming that index is created by the statement
CREATE INDEX idx ON ab (a) INCLUDING (b);
Sorry, I forgot the index, and yes you guessed correctly about that.
The problem only exists without the omit_opclass_4.0.patch and with the
covering_unique_4.0.patch, so please ignore.
I will try to review the omit_opclass_4.0.patch soon.
David
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
