Partitioned tables and covering indexes
Hi,
Trying covering indexes on partitioned tables i get this error
"""
postgres=# create index on t1_part (i) include (t);
ERROR: cache lookup failed for opclass 0
"""
To reproduce:
create table t1_part (i int, t text) partition by hash (i);
create table t1_part_0 partition of t1_part for values with (modulus
2, remainder 0);
create table t1_part_1 partition of t1_part for values with (modulus
2, remainder 1);
insert into t1_part values (1, repeat('abcdefquerty', 20));
create index on t1_part (i) include (t);
--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/04/10 16:07, Jaime Casanova wrote:
Hi,
Trying covering indexes on partitioned tables i get this error
"""
postgres=# create index on t1_part (i) include (t);
ERROR: cache lookup failed for opclass 0
"""To reproduce:
create table t1_part (i int, t text) partition by hash (i);
create table t1_part_0 partition of t1_part for values with (modulus
2, remainder 0);
create table t1_part_1 partition of t1_part for values with (modulus
2, remainder 1);
insert into t1_part values (1, repeat('abcdefquerty', 20));create index on t1_part (i) include (t);
It seems that the bug is caused due to the original IndexStmt that
DefineIndex receives being overwritten when processing the INCLUDE
columns. Also, the original copy of it should be used when recursing for
defining the index in partitions.
Does the attached fix look correct? Haven't checked the fix with ATTACH
PARTITION though.
Maybe add this to open items list?
Thanks,
Amit
Attachments:
DefineIndex-fix-covering-index-partitioned-1.patchtext/plain; charset=UTF-8; name=DefineIndex-fix-covering-index-partitioned-1.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 860a60d109..109d48bb2f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -366,6 +366,7 @@ DefineIndex(Oid relationId,
LOCKMODE lockmode;
Snapshot snapshot;
int i;
+ IndexStmt *orig_stmt = copyObject(stmt);
if (list_intersection(stmt->indexParams, stmt->indexIncludingParams) != NIL)
ereport(ERROR,
@@ -886,8 +887,8 @@ DefineIndex(Oid relationId,
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
- opfamOids = palloc(sizeof(Oid) * numberOfAttributes);
- for (i = 0; i < numberOfAttributes; i++)
+ opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
+ for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
heap_close(rel, NoLock);
@@ -987,11 +988,11 @@ DefineIndex(Oid relationId,
*/
if (!found)
{
- IndexStmt *childStmt = copyObject(stmt);
+ IndexStmt *childStmt = copyObject(orig_stmt);
bool found_whole_row;
childStmt->whereClause =
- map_variable_attnos(stmt->whereClause, 1, 0,
+ map_variable_attnos(orig_stmt->whereClause, 1, 0,
attmap, maplen,
InvalidOid, &found_whole_row);
if (found_whole_row)
On Tue, Apr 10, 2018 at 12:47 PM, Amit Langote <
Langote_Amit_f8@lab.ntt.co.jp> wrote:
On 2018/04/10 16:07, Jaime Casanova wrote:
Hi,
Trying covering indexes on partitioned tables i get this error
"""
postgres=# create index on t1_part (i) include (t);
ERROR: cache lookup failed for opclass 0
"""To reproduce:
create table t1_part (i int, t text) partition by hash (i);
create table t1_part_0 partition of t1_part for values with (modulus
2, remainder 0);
create table t1_part_1 partition of t1_part for values with (modulus
2, remainder 1);
insert into t1_part values (1, repeat('abcdefquerty', 20));create index on t1_part (i) include (t);
It seems that the bug is caused due to the original IndexStmt that
DefineIndex receives being overwritten when processing the INCLUDE
columns. Also, the original copy of it should be used when recursing for
defining the index in partitions.Does the attached fix look correct? Haven't checked the fix with ATTACH
PARTITION though.
Attached patch seems to fix the problem. However, I would rather get
rid of modifying stmt->indexParams. That seems to be more logical
for me. Also, it would be good to check some covering indexes on
partitioned tables. See the attached patch.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
DefineIndex-fix-covering-index-partitioned-2.patchapplication/octet-stream; name=DefineIndex-fix-covering-index-partitioned-2.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 860a60d109..ad819177d7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -342,6 +342,7 @@ DefineIndex(Oid relationId,
Oid tablespaceId;
Oid createdConstraintId = InvalidOid;
List *indexColNames;
+ List *allIndexParams;
Relation rel;
Relation indexRelation;
HeapTuple tuple;
@@ -378,16 +379,16 @@ DefineIndex(Oid relationId,
numberOfKeyAttributes = list_length(stmt->indexParams);
/*
- * We append any INCLUDE columns onto the indexParams list so that we have
- * one list with all columns. Later we can determine which of these are
- * key columns, and which are just part of the INCLUDE list by checking
- * the list position. A list item in a position less than
- * ii_NumIndexKeyAttrs is part of the key columns, and anything equal to
- * and over is part of the INCLUDE columns.
+ * Calculate the new list of index columns including both key columns and
+ * INCLUDE columns. Later we can determine which of these are key columns,
+ * and which are just part of the INCLUDE list by checking the list
+ * position. A list item in a position less than ii_NumIndexKeyAttrs is
+ * part of the key columns, and anything equal to and over is part of the
+ * INCLUDE columns.
*/
- stmt->indexParams = list_concat(stmt->indexParams,
- stmt->indexIncludingParams);
- numberOfAttributes = list_length(stmt->indexParams);
+ allIndexParams = list_concat(list_copy(stmt->indexParams),
+ list_copy(stmt->indexIncludingParams));
+ numberOfAttributes = list_length(allIndexParams);
if (numberOfAttributes <= 0)
ereport(ERROR,
@@ -544,7 +545,7 @@ DefineIndex(Oid relationId,
/*
* Choose the index column names.
*/
- indexColNames = ChooseIndexColumnNames(stmt->indexParams);
+ indexColNames = ChooseIndexColumnNames(allIndexParams);
/*
* Select name for index if caller didn't specify
@@ -658,7 +659,7 @@ DefineIndex(Oid relationId,
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
ComputeIndexAttrs(indexInfo,
typeObjectId, collationObjectId, classObjectId,
- coloptions, stmt->indexParams,
+ coloptions, allIndexParams,
stmt->excludeOpNames, relationId,
accessMethodName, accessMethodId,
amcanorder, stmt->isconstraint);
@@ -886,8 +887,8 @@ DefineIndex(Oid relationId,
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
- opfamOids = palloc(sizeof(Oid) * numberOfAttributes);
- for (i = 0; i < numberOfAttributes; i++)
+ opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
+ for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
heap_close(rel, NoLock);
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 33f68aab71..1ff57c70c6 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1038,19 +1038,19 @@ create table idxpart1 partition of idxpart for values from (0) to (100000);
create table idxpart2 (c int, like idxpart);
insert into idxpart2 (c, a, b) values (42, 572814, 'inserted first');
alter table idxpart2 drop column c;
-create unique index on idxpart (a);
+create unique index on idxpart (a) include (b);
alter table idxpart attach partition idxpart2 for values from (100000) to (1000000);
insert into idxpart values (0, 'zero'), (42, 'life'), (2^16, 'sixteen');
insert into idxpart select 2^g, format('two to power of %s', g) from generate_series(15, 17) g;
-ERROR: duplicate key value violates unique constraint "idxpart1_a_idx"
+ERROR: duplicate key value violates unique constraint "idxpart1_a_b_idx"
DETAIL: Key (a)=(65536) already exists.
insert into idxpart values (16, 'sixteen');
insert into idxpart (b, a) values ('one', 142857), ('two', 285714);
insert into idxpart select a * 2, b || b from idxpart where a between 2^16 and 2^19;
-ERROR: duplicate key value violates unique constraint "idxpart2_a_idx"
+ERROR: duplicate key value violates unique constraint "idxpart2_a_b_idx"
DETAIL: Key (a)=(285714) already exists.
insert into idxpart values (572814, 'five');
-ERROR: duplicate key value violates unique constraint "idxpart2_a_idx"
+ERROR: duplicate key value violates unique constraint "idxpart2_a_b_idx"
DETAIL: Key (a)=(572814) already exists.
insert into idxpart values (857142, 'six');
select tableoid::regclass, * from idxpart order by a;
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index ab7c2d1475..9aa2784e9c 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -562,7 +562,7 @@ create table idxpart1 partition of idxpart for values from (0) to (100000);
create table idxpart2 (c int, like idxpart);
insert into idxpart2 (c, a, b) values (42, 572814, 'inserted first');
alter table idxpart2 drop column c;
-create unique index on idxpart (a);
+create unique index on idxpart (a) include (b);
alter table idxpart attach partition idxpart2 for values from (100000) to (1000000);
insert into idxpart values (0, 'zero'), (42, 'life'), (2^16, 'sixteen');
insert into idxpart select 2^g, format('two to power of %s', g) from generate_series(15, 17) g;
Does the attached fix look correct?О©╫ Haven't checked the fix with ATTACH
PARTITION though.Attached patch seems to fix the problem.О©╫ However, I would rather get
rid of modifyingО©╫stmt->indexParams.О©╫ That seems to be more logical
for me.О©╫ Also, it would be good to check some covering indexes on
partitioned tables.О©╫ See the attached patch.
Seems right way, do not modify incoming object and do not copy rather large and
deep nested structure as suggested by Amit.
But it will be better to have a ATTACH PARTITION test too.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
On 10 April 2018 at 10:36, Teodor Sigaev <teodor@sigaev.ru> wrote:
Does the attached fix look correct? Haven't checked the fix with
ATTACH
PARTITION though.Attached patch seems to fix the problem. However, I would rather get
rid of modifying stmt->indexParams. That seems to be more logical
for me. Also, it would be good to check some covering indexes on
partitioned tables. See the attached patch.Seems right way, do not modify incoming object and do not copy rather large
and deep nested structure as suggested by Amit.But it will be better to have a ATTACH PARTITION test too.
the patch worked for me, i also tried some combinations using ATTACH
PARTITION and found no problems
--
Jaime Casanova www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi.
On 2018/04/11 0:36, Teodor Sigaev wrote:
О©╫О©╫О©╫ Does the attached fix look correct?О©╫ Haven't checked the fix with
ATTACH
О©╫О©╫О©╫ PARTITION though.Attached patch seems to fix the problem.О©╫ However, I would rather get
rid of modifyingО©╫stmt->indexParams.О©╫ That seems to be more logical
for me.О©╫ Also, it would be good to check some covering indexes on
partitioned tables.О©╫ See the attached patch.Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.
Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.
But it willО©╫ be better to have a ATTACH PARTITION test too.
I have added tests. Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.
Attached find updated patch.
Thanks,
Amit
Attachments:
DefineIndex-fix-covering-index-partitioned-3.patchtext/plain; charset=UTF-8; name=DefineIndex-fix-covering-index-partitioned-3.patchDownload
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 860a60d109..ad819177d7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -342,6 +342,7 @@ DefineIndex(Oid relationId,
Oid tablespaceId;
Oid createdConstraintId = InvalidOid;
List *indexColNames;
+ List *allIndexParams;
Relation rel;
Relation indexRelation;
HeapTuple tuple;
@@ -378,16 +379,16 @@ DefineIndex(Oid relationId,
numberOfKeyAttributes = list_length(stmt->indexParams);
/*
- * We append any INCLUDE columns onto the indexParams list so that we have
- * one list with all columns. Later we can determine which of these are
- * key columns, and which are just part of the INCLUDE list by checking
- * the list position. A list item in a position less than
- * ii_NumIndexKeyAttrs is part of the key columns, and anything equal to
- * and over is part of the INCLUDE columns.
+ * Calculate the new list of index columns including both key columns and
+ * INCLUDE columns. Later we can determine which of these are key columns,
+ * and which are just part of the INCLUDE list by checking the list
+ * position. A list item in a position less than ii_NumIndexKeyAttrs is
+ * part of the key columns, and anything equal to and over is part of the
+ * INCLUDE columns.
*/
- stmt->indexParams = list_concat(stmt->indexParams,
- stmt->indexIncludingParams);
- numberOfAttributes = list_length(stmt->indexParams);
+ allIndexParams = list_concat(list_copy(stmt->indexParams),
+ list_copy(stmt->indexIncludingParams));
+ numberOfAttributes = list_length(allIndexParams);
if (numberOfAttributes <= 0)
ereport(ERROR,
@@ -544,7 +545,7 @@ DefineIndex(Oid relationId,
/*
* Choose the index column names.
*/
- indexColNames = ChooseIndexColumnNames(stmt->indexParams);
+ indexColNames = ChooseIndexColumnNames(allIndexParams);
/*
* Select name for index if caller didn't specify
@@ -658,7 +659,7 @@ DefineIndex(Oid relationId,
coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16));
ComputeIndexAttrs(indexInfo,
typeObjectId, collationObjectId, classObjectId,
- coloptions, stmt->indexParams,
+ coloptions, allIndexParams,
stmt->excludeOpNames, relationId,
accessMethodName, accessMethodId,
amcanorder, stmt->isconstraint);
@@ -886,8 +887,8 @@ DefineIndex(Oid relationId,
memcpy(part_oids, partdesc->oids, sizeof(Oid) * nparts);
parentDesc = CreateTupleDescCopy(RelationGetDescr(rel));
- opfamOids = palloc(sizeof(Oid) * numberOfAttributes);
- for (i = 0; i < numberOfAttributes; i++)
+ opfamOids = palloc(sizeof(Oid) * numberOfKeyAttributes);
+ for (i = 0; i < numberOfKeyAttributes; i++)
opfamOids[i] = get_opclass_family(classObjectId[i]);
heap_close(rel, NoLock);
diff --git a/src/test/regress/expected/indexing.out b/src/test/regress/expected/indexing.out
index 33f68aab71..2c2bf44aa8 100644
--- a/src/test/regress/expected/indexing.out
+++ b/src/test/regress/expected/indexing.out
@@ -1313,3 +1313,27 @@ alter index idxpart2_a_idx attach partition idxpart22_a_idx;
create index on idxpart (a);
create table idxpart_another (a int, b int, primary key (a, b)) partition by range (a);
create table idxpart_another_1 partition of idxpart_another for values from (0) to (100);
+-- Test that covering partitioned indexes work in various cases
+create table covidxpart (a int, b int) partition by list (a);
+create unique index on covidxpart (a) include (b);
+create table covidxpart1 partition of covidxpart for values in (1);
+create table covidxpart2 partition of covidxpart for values in (2);
+insert into covidxpart values (1, 1);
+insert into covidxpart values (1, 1);
+ERROR: duplicate key value violates unique constraint "covidxpart1_a_b_idx"
+DETAIL: Key (a)=(1) already exists.
+create table covidxpart3 (b int, c int, a int);
+alter table covidxpart3 drop c;
+alter table covidxpart attach partition covidxpart3 for values in (3);
+insert into covidxpart values (3, 1);
+insert into covidxpart values (3, 1);
+ERROR: duplicate key value violates unique constraint "covidxpart3_a_b_idx"
+DETAIL: Key (a)=(3) already exists.
+create table covidxpart4 (b int, a int);
+create unique index on covidxpart4 (a) include (b);
+create unique index on covidxpart4 (a);
+alter table covidxpart attach partition covidxpart4 for values in (4);
+insert into covidxpart values (4, 1);
+insert into covidxpart values (4, 1);
+ERROR: duplicate key value violates unique constraint "covidxpart4_a_b_idx"
+DETAIL: Key (a)=(4) already exists.
diff --git a/src/test/regress/sql/indexing.sql b/src/test/regress/sql/indexing.sql
index ab7c2d1475..29333b31ef 100644
--- a/src/test/regress/sql/indexing.sql
+++ b/src/test/regress/sql/indexing.sql
@@ -701,3 +701,22 @@ alter index idxpart2_a_idx attach partition idxpart22_a_idx;
create index on idxpart (a);
create table idxpart_another (a int, b int, primary key (a, b)) partition by range (a);
create table idxpart_another_1 partition of idxpart_another for values from (0) to (100);
+
+-- Test that covering partitioned indexes work in various cases
+create table covidxpart (a int, b int) partition by list (a);
+create unique index on covidxpart (a) include (b);
+create table covidxpart1 partition of covidxpart for values in (1);
+create table covidxpart2 partition of covidxpart for values in (2);
+insert into covidxpart values (1, 1);
+insert into covidxpart values (1, 1);
+create table covidxpart3 (b int, c int, a int);
+alter table covidxpart3 drop c;
+alter table covidxpart attach partition covidxpart3 for values in (3);
+insert into covidxpart values (3, 1);
+insert into covidxpart values (3, 1);
+create table covidxpart4 (b int, a int);
+create unique index on covidxpart4 (a) include (b);
+create unique index on covidxpart4 (a);
+alter table covidxpart attach partition covidxpart4 for values in (4);
+insert into covidxpart values (4, 1);
+insert into covidxpart values (4, 1);
Thank you, pushed
Amit Langote wrote:
Hi.
On 2018/04/11 0:36, Teodor Sigaev wrote:
О©╫О©╫О©╫ Does the attached fix look correct?О©╫ Haven't checked the fix with
ATTACH
О©╫О©╫О©╫ PARTITION though.Attached patch seems to fix the problem.О©╫ However, I would rather get
rid of modifyingО©╫stmt->indexParams.О©╫ That seems to be more logical
for me.О©╫ Also, it would be good to check some covering indexes on
partitioned tables.О©╫ See the attached patch.Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.But it willО©╫ be better to have a ATTACH PARTITION test too.
I have added tests. Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.Attached find updated patch.
Thanks,
Amit
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Patch makes buildfarm almost red, patch is temporary reverted.
Actually, discovered bug is not related to patch except new test faces with it,
problem is: CompareIndexInfo() checks rd_opfamily for equality for all
attributes, not only for key attribute.
Obviously, CompareIndexInfo() needs more work:
for (i = 0; i < info1->ii_NumIndexAttrs; i++)
{
if (maplen < info2->ii_KeyAttrNumbers[i])
Seems, we can go out from ii_KeyAttrNumbers array.
Amit Langote wrote:
Hi.
On 2018/04/11 0:36, Teodor Sigaev wrote:
О©╫О©╫О©╫ Does the attached fix look correct?О©╫ Haven't checked the fix with
ATTACH
О©╫О©╫О©╫ PARTITION though.Attached patch seems to fix the problem.О©╫ However, I would rather get
rid of modifyingО©╫stmt->indexParams.О©╫ That seems to be more logical
for me.О©╫ Also, it would be good to check some covering indexes on
partitioned tables.О©╫ See the attached patch.Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.But it willО©╫ be better to have a ATTACH PARTITION test too.
I have added tests. Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.Attached find updated patch.
Thanks,
Amit
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Actually, discovered bug is not related to patch except new test faces
with it,
problem is: CompareIndexInfo() checks rd_opfamily for equality for all
attributes, not only for key attribute.
Patch attached. But it seems to me, field's names of
IndexInfo structure are a bit confused now:
int ii_NumIndexAttrs; /* total number of columns in index */
int ii_NumIndexKeyAttrs; /* number of key columns in
index */
AttrNumber ii_KeyAttrNumbers[INDEX_MAX_KEYS];
ii_KeyAttrNumbers contains all columns, i.e. it contains
ii_NumIndexAttrs number of columns, not a ii_NumIndexKeyAttrs number as
easy to think.
I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or
ii_IndexAttrNumbers. Opinions?
О©╫О©╫О©╫ for (i = 0; i < info1->ii_NumIndexAttrs; i++)
О©╫О©╫О©╫ {
О©╫О©╫О©╫О©╫О©╫О©╫О©╫ if (maplen < info2->ii_KeyAttrNumbers[i])
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Attachments:
CompareIndexInfo.patchtext/x-patch; name=CompareIndexInfo.patchDownload
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5d73e92901..0002816fcc 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1831,6 +1831,10 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
if (info1->ii_NumIndexAttrs != info2->ii_NumIndexAttrs)
return false;
+ /* and same number of key attributes */
+ if (info1->ii_NumIndexKeyAttrs != info2->ii_NumIndexKeyAttrs)
+ return false;
+
/*
* and columns match through the attribute map (actual attribute numbers
* might differ!) Note that this implies that index columns that are
@@ -1850,7 +1854,9 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
if (collations1[i] != collations2[i])
return false;
- if (opfamilies1[i] != opfamilies2[i])
+
+ /* opfamily is valid on for key attributes */
+ if (i < info2->ii_NumIndexKeyAttrs && opfamilies1[i] != opfamilies2[i])
return false;
}
Teodor Sigaev wrote:
Patch attached.
I wonder why this is a problem in opfamilies but not collations.
If we don't compare collations, wouldn't it make more sense to break out
of the loop once the number of keys is reached?
When this code was written, there was no question as to what length the
opfamilies/collations the arrays were; it was obvious that they must be
of the length of the index attributes. It's not obvious now. Maybe add
a comment about that?
But it seems to me, field's names of
IndexInfo structure are a bit confused now:
int ii_NumIndexAttrs; /* total number of columns in index */
int ii_NumIndexKeyAttrs; /* number of key columns in index */
AttrNumber ii_KeyAttrNumbers[INDEX_MAX_KEYS];ii_KeyAttrNumbers contains all columns, i.e. it contains ii_NumIndexAttrs
number of columns, not a ii_NumIndexKeyAttrs number as easy to think.I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or ii_IndexAttrNumbers.
Opinions?
Yeah, the current situation seems very odd.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 11, 2018 at 10:47 PM, Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
Teodor Sigaev wrote:
Patch attached.
I wonder why this is a problem in opfamilies but not collations.
If we don't compare collations, wouldn't it make more sense to break out
of the loop once the number of keys is reached?
It appears that INCLUDE columns might have collation defined.
For instance, following query is working:
create index t_s1_s2_idx on t (s1) include (s2 collate "en_US.UTF-8");
However, I don't see any point in defining collations here, because
INCLUDE attributes exist solely for index-only scans. So, index just
can return value of INCLUDE attribute "as is", no point to do something
with collation.
So, I propose to disable collations for INCLUDE attributes.
When this code was written, there was no question as to what length the
opfamilies/collations the arrays were; it was obvious that they must be
of the length of the index attributes. It's not obvious now. Maybe add
a comment about that?
In b3b7f789 Tom have resized one array size from total number of
attributes to number of key attributes. And I like idea of applying the
same to all other array: make them sized to total number of attributes
with filling of absent attributes with 0.
But it seems to me, field's names of
IndexInfo structure are a bit confused now:
int ii_NumIndexAttrs; /* total number of columns in index*/
int ii_NumIndexKeyAttrs; /* number of key columns in
index */
AttrNumber ii_KeyAttrNumbers[INDEX_MAX_KEYS];
ii_KeyAttrNumbers contains all columns, i.e. it contains ii_NumIndexAttrs
number of columns, not a ii_NumIndexKeyAttrs number as easy to think.I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or
ii_IndexAttrNumbers.
Opinions?
Yeah, the current situation seems very odd.
+1 for ii_IndexAttrNumbers.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Wed, Apr 11, 2018 at 1:58 PM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
It appears that INCLUDE columns might have collation defined.
For instance, following query is working:create index t_s1_s2_idx on t (s1) include (s2 collate "en_US.UTF-8");
However, I don't see any point in defining collations here, because
INCLUDE attributes exist solely for index-only scans. So, index just
can return value of INCLUDE attribute "as is", no point to do something
with collation.So, I propose to disable collations for INCLUDE attributes.
Hmm. I'm not sure that that's exactly the right thing to do. We seem
to want to have case-insensitive collations in the future. The fact
that you can spell out collation name in ON CONFLICT's unique index
inference specification suggests this, for example. I think that a
collation is theoretically allowed to affect the behavior of equality,
even though so far we've never tried to make that work for any
collatable datatype.
Maybe the right thing to do is to say that any collation will work
equally well (as will any opclass). Maybe that's the same thing as
what you said, though.
+1 for ii_IndexAttrNumbers.
+1
--
Peter Geoghegan
On 4/11/18 17:08, Peter Geoghegan wrote:
However, I don't see any point in defining collations here, because
INCLUDE attributes exist solely for index-only scans. So, index just
can return value of INCLUDE attribute "as is", no point to do something
with collation.So, I propose to disable collations for INCLUDE attributes.
Hmm. I'm not sure that that's exactly the right thing to do. We seem
to want to have case-insensitive collations in the future. The fact
that you can spell out collation name in ON CONFLICT's unique index
inference specification suggests this, for example. I think that a
collation is theoretically allowed to affect the behavior of equality,
even though so far we've never tried to make that work for any
collatable datatype.
But in this case it doesn't even do equality comparison, it just returns
the value.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 11, 2018 at 2:29 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
But in this case it doesn't even do equality comparison, it just returns
the value.
That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.
--
Peter Geoghegan
On 4/11/18 17:38, Peter Geoghegan wrote:
On Wed, Apr 11, 2018 at 2:29 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:But in this case it doesn't even do equality comparison, it just returns
the value.That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.
Or maybe it should be the collation of the underlying table columns.
Otherwise the collation returned by an index-only scan would be
different from a table scan, no?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Geoghegan wrote:
On Wed, Apr 11, 2018 at 2:29 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:But in this case it doesn't even do equality comparison, it just returns
the value.That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.
Any operation over including columns is a deal for filter, not an index,
so collation will not be used somewhere inside index.
2Alexander: ComputeIndexAttrs() may use whole vectors (for including
columns too) just to simplify coding, in other places, seems, better to
have exact size of vectors. Using full-sized vectors could mask a error,
for exmaple, with setting opfamily to InvalidOid for key column. But if
you insist on that, I'll modify attached patch to use full-sized vectors
anywhere.
In attached path I did:
1) fix a bug in CompareIndexInfo() which checks opfamily for including
column
2) correct size for all vectors
3) Add assertion in various places to be sure that we don't try to use
including column as key column
4) per Peter Geoghegan idea add a error message if somebody adds options
to include column instead silently ignore it.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
Attachments:
CompareIndexInfo-v2.patchtext/x-patch; name=CompareIndexInfo-v2.patchDownload
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5d73e92901..58b4249784 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -297,6 +297,7 @@ ConstructTupleDescriptor(Relation heapRelation,
Oid *classObjectId)
{
int numatts = indexInfo->ii_NumIndexAttrs;
+ int numkeyatts = indexInfo->ii_NumIndexKeyAttrs;
ListCell *colnames_item = list_head(indexColNames);
ListCell *indexpr_item = list_head(indexInfo->ii_Expressions);
IndexAmRoutine *amroutine;
@@ -375,7 +376,8 @@ ConstructTupleDescriptor(Relation heapRelation,
to->attidentity = '\0';
to->attislocal = true;
to->attinhcount = 0;
- to->attcollation = collationObjectId[i];
+ to->attcollation = (i < numkeyatts) ?
+ collationObjectId[i] : InvalidOid;
}
else
{
@@ -411,7 +413,8 @@ ConstructTupleDescriptor(Relation heapRelation,
to->attcacheoff = -1;
to->atttypmod = exprTypmod(indexkey);
to->attislocal = true;
- to->attcollation = collationObjectId[i];
+ to->attcollation = (i < numkeyatts) ?
+ collationObjectId[i] : InvalidOid;
ReleaseSysCache(tuple);
@@ -608,9 +611,9 @@ UpdateIndexRelation(Oid indexoid,
indkey = buildint2vector(NULL, indexInfo->ii_NumIndexAttrs);
for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
indkey->values[i] = indexInfo->ii_KeyAttrNumbers[i];
- indcollation = buildoidvector(collationOids, indexInfo->ii_NumIndexAttrs);
+ indcollation = buildoidvector(collationOids, indexInfo->ii_NumIndexKeyAttrs);
indclass = buildoidvector(classOids, indexInfo->ii_NumIndexKeyAttrs);
- indoption = buildint2vector(coloptions, indexInfo->ii_NumIndexAttrs);
+ indoption = buildint2vector(coloptions, indexInfo->ii_NumIndexKeyAttrs);
/*
* Convert the index expressions (if any) to a text datum
@@ -1080,7 +1083,7 @@ index_create(Relation heapRelation,
/* Store dependency on collations */
/* The default collation is pinned, so don't bother recording it */
- for (i = 0; i < indexInfo->ii_NumIndexAttrs; i++)
+ for (i = 0; i < indexInfo->ii_NumIndexKeyAttrs; i++)
{
if (OidIsValid(collationObjectId[i]) &&
collationObjectId[i] != DEFAULT_COLLATION_OID)
@@ -1831,6 +1834,11 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
if (info1->ii_NumIndexAttrs != info2->ii_NumIndexAttrs)
return false;
+ /* and same number of key attributes */
+ if (info1->ii_NumIndexKeyAttrs != info2->ii_NumIndexKeyAttrs)
+ return false;
+
+
/*
* and columns match through the attribute map (actual attribute numbers
* might differ!) Note that this implies that index columns that are
@@ -1848,6 +1856,10 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
info1->ii_KeyAttrNumbers[i]))
return false;
+ /* collation and opfamily is not valid for including columns */
+ if (i >= info1->ii_NumIndexKeyAttrs)
+ continue;
+
if (collations1[i] != collations2[i])
return false;
if (opfamilies1[i] != opfamilies2[i])
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 860a60d109..4933608e61 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1359,7 +1359,8 @@ CheckPredicate(Expr *predicate)
/*
* Compute per-index-column information, including indexed column numbers
- * or index expressions, opclasses, and indoptions.
+ * or index expressions, opclasses, and indoptions. Note, all output vectors
+ * should be allocated for all columns, including "including" ones.
*/
static void
ComputeIndexAttrs(IndexInfo *indexInfo,
@@ -1490,6 +1491,36 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
typeOidP[attn] = atttype;
+ /*
+ * Included columns have no collation, no opclass and no ordering options.
+ */
+ if (attn >= nkeycols)
+ {
+ if (attribute->collation)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("including column does not support a collation")));
+ if (attribute->opclass)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("including column does not support an operator class")));
+ if (attribute->ordering != SORTBY_DEFAULT)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("including column does not support ASC/DESC options")));
+ if (attribute->nulls_ordering != SORTBY_NULLS_DEFAULT)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("including column does not support NULLS FIRST/LAST options")));
+
+ classOidP[attn] = InvalidOid;
+ colOptionP[attn] = 0;
+ collationOidP[attn] = InvalidOid;
+ attn++;
+
+ continue;
+ }
+
/*
* Apply collation override if any
*/
@@ -1521,17 +1552,6 @@ ComputeIndexAttrs(IndexInfo *indexInfo,
collationOidP[attn] = attcollation;
- /*
- * Included columns have no opclass and no ordering options.
- */
- if (attn >= nkeycols)
- {
- classOidP[attn] = InvalidOid;
- colOptionP[attn] = 0;
- attn++;
- continue;
- }
-
/*
* Identify the opclass to use.
*/
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index bf42b54970..72dfe3f6d6 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -2339,6 +2339,8 @@ match_clause_to_indexcol(IndexOptInfo *index,
Oid expr_coll;
bool plain_op;
+ Assert(indexcol < index->nkeycolumns);
+
/* First check for boolean-index cases. */
if (IsBooleanOpfamily(opfamily))
{
@@ -2687,6 +2689,8 @@ match_clause_to_ordering_op(IndexOptInfo *index,
Oid sortfamily;
bool commuted;
+ Assert(indexcol < index->nkeycolumns);
+
/*
* Clause must be a binary opclause.
*/
@@ -2924,6 +2928,8 @@ ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel,
Oid curFamily = index->opfamily[indexcol];
Oid curCollation = index->indexcollations[indexcol];
+ Assert(indexcol < index->nkeycolumns);
+
/*
* If it's a btree index, we can reject it if its opfamily isn't
* compatible with the EC, since no clause generated from the EC could be
@@ -3551,6 +3557,8 @@ expand_indexqual_conditions(IndexOptInfo *index,
Oid curFamily = index->opfamily[indexcol];
Oid curCollation = index->indexcollations[indexcol];
+ Assert(indexcol < index->nkeycolumns);
+
/* First check for boolean cases */
if (IsBooleanOpfamily(curFamily))
{
@@ -3918,14 +3926,15 @@ adjust_rowcompare_for_index(RowCompareExpr *clause,
/*
* The Var side can match any column of the index.
*/
- for (i = 0; i < index->ncolumns; i++)
+ for (i = 0; i < index->nkeycolumns; i++)
{
if (match_index_to_operand(varop, i, index) &&
get_op_opfamily_strategy(expr_op,
index->opfamily[i]) == op_strategy &&
IndexCollMatchesExprColl(index->indexcollations[i],
lfirst_oid(collids_cell)))
- break;
+
+ break;
}
if (i >= index->ncolumns)
break; /* no match found */
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 90bb0c2804..7f94e6ca67 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -242,7 +242,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
info->nkeycolumns = nkeycolumns = index->indnkeyatts;
info->indexkeys = (int *) palloc(sizeof(int) * ncolumns);
- info->indexcollations = (Oid *) palloc(sizeof(Oid) * ncolumns);
+ info->indexcollations = (Oid *) palloc(sizeof(Oid) * nkeycolumns);
info->opfamily = (Oid *) palloc(sizeof(Oid) * nkeycolumns);
info->opcintype = (Oid *) palloc(sizeof(Oid) * nkeycolumns);
info->canreturn = (bool *) palloc(sizeof(bool) * ncolumns);
@@ -250,7 +250,6 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
for (i = 0; i < ncolumns; i++)
{
info->indexkeys[i] = index->indkey.values[i];
- info->indexcollations[i] = indexRelation->rd_indcollation[i];
info->canreturn[i] = index_can_return(indexRelation, i + 1);
}
@@ -258,6 +257,7 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
{
info->opfamily[i] = indexRelation->rd_opfamily[i];
info->opcintype[i] = indexRelation->rd_opcintype[i];
+ info->indexcollations[i] = indexRelation->rd_indcollation[i];
}
info->relam = indexRelation->rd_rel->relam;
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index f9f9904bad..9178139912 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -1588,9 +1588,6 @@ generateClonedIndexStmt(RangeVar *heapRel, Oid heapRelid, Relation source_idx,
/* Copy the original index column name */
iparam->indexcolname = pstrdup(NameStr(attr->attname));
- /* Add the collation name, if non-default */
- iparam->collation = get_collation(indcollation->values[keyno], keycoltype);
-
index->indexIncludingParams = lappend(index->indexIncludingParams, iparam);
}
/* Copy reloptions if any */
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b75a224ee8..99643e83b2 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1356,15 +1356,15 @@ pg_get_indexdef_worker(Oid indexrelid, int colno,
{
Oid indcoll;
+ if (keyno >= idxrec->indnkeyatts)
+ continue;
+
/* Add collation, if not default for column */
indcoll = indcollation->values[keyno];
if (OidIsValid(indcoll) && indcoll != keycolcollation)
appendStringInfo(&buf, " COLLATE %s",
generate_collation_name((indcoll)));
- if (keyno >= idxrec->indnkeyatts)
- continue;
-
/* Add the operator class name, if not default */
get_opclass_name(indclass->values[keyno], keycoltype, &buf);
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index fe606d7279..4b08cdb721 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -7437,6 +7437,8 @@ gincost_pattern(IndexOptInfo *index, int indexcol,
int32 searchMode = GIN_SEARCH_MODE_DEFAULT;
int32 i;
+ Assert(indexcol < index->nkeycolumns);
+
/*
* Get the operator's strategy number and declared input data types within
* the index opfamily. (We don't need the latter, but we use
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e81c4691ec..0cdfa5d9e4 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -1637,10 +1637,10 @@ RelationInitIndexAccessInfo(Relation relation)
}
relation->rd_indcollation = (Oid *)
- MemoryContextAllocZero(indexcxt, indnatts * sizeof(Oid));
+ MemoryContextAllocZero(indexcxt, indnkeyatts * sizeof(Oid));
relation->rd_indoption = (int16 *)
- MemoryContextAllocZero(indexcxt, indnatts * sizeof(int16));
+ MemoryContextAllocZero(indexcxt, indnkeyatts * sizeof(int16));
/*
* indcollation cannot be referenced directly through the C struct,
@@ -1653,7 +1653,7 @@ RelationInitIndexAccessInfo(Relation relation)
&isnull);
Assert(!isnull);
indcoll = (oidvector *) DatumGetPointer(indcollDatum);
- memcpy(relation->rd_indcollation, indcoll->values, indnatts * sizeof(Oid));
+ memcpy(relation->rd_indcollation, indcoll->values, indnkeyatts * sizeof(Oid));
/*
* indclass cannot be referenced directly through the C struct, because it
@@ -1685,7 +1685,7 @@ RelationInitIndexAccessInfo(Relation relation)
&isnull);
Assert(!isnull);
indoption = (int2vector *) DatumGetPointer(indoptionDatum);
- memcpy(relation->rd_indoption, indoption->values, indnatts * sizeof(int16));
+ memcpy(relation->rd_indoption, indoption->values, indnkeyatts * sizeof(int16));
/*
* expressions, predicate, exclusion caches will be filled later
That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.Or maybe it should be the collation of the underlying table columns.
Otherwise the collation returned by an index-only scan would be
different from a table scan, no?
+1, dangerous
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/On Thu, Apr 12, 2018 at 1:14 AM, Teodor Sigaev <teodor@sigaev.ru> wrote:
That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.Or maybe it should be the collation of the underlying table columns.
Otherwise the collation returned by an index-only scan would be
different from a table scan, no?+1, dangerous
I'm OK with collation of included columns to be the same as collation
of underlying table columns. But I still think we should throw an error
when user is trying to specify his own collation of included columns.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Thu, Apr 12, 2018 at 1:14 AM, Teodor Sigaev <teodor@sigaev.ru> wrote:
Peter Geoghegan wrote:
On Wed, Apr 11, 2018 at 2:29 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:But in this case it doesn't even do equality comparison, it just returns
the value.That's the idea that I tried to express. The point is that we need to
tell the user that there is no need to worry about it, rather than
that they're wrong to ask about it. Though we should probably actually
just throw an error.Any operation over including columns is a deal for filter, not an index,
so collation will not be used somewhere inside index.2Alexander: ComputeIndexAttrs() may use whole vectors (for including
columns too) just to simplify coding, in other places, seems, better to
have exact size of vectors. Using full-sized vectors could mask a error,
for exmaple, with setting opfamily to InvalidOid for key column. But if you
insist on that, I'll modify attached patch to use full-sized vectors
anywhere.In attached path I did:
1) fix a bug in CompareIndexInfo() which checks opfamily for including
column
2) correct size for all vectors
3) Add assertion in various places to be sure that we don't try to use
including column as key column
4) per Peter Geoghegan idea add a error message if somebody adds options
to include column instead silently ignore it.
I don't insist on using full-sized vectors everywhere. Attached patch
looks OK for me. I haven't test it though.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Thanks to everyone, this part is pushed. I will waiting a bit before pushing
topic-stater patch to have a time to look on buildfarm.
Alvaro, could you add a comment to CompareIndexInfo() to clarify why it doesn't
compare indoptions (like DESC/ASC etc)? It doesn't matter for uniqueness of
index but changes an order and it's not clear at least for me why we don't pay
attention for that.
In attached path I did:
1) fix a bug in CompareIndexInfo() which checks opfamily for including column
2) correct size for all vectors
3) Add assertion in various places to be sure that we don't try to use including
column as key column
4) per Peter Geoghegan idea add a error message if somebody adds options to
include column instead silently ignore it.
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
pushed. Hope, second try will be successful.
Teodor Sigaev wrote:
Thank you, pushed
Amit Langote wrote:
Hi.
On 2018/04/11 0:36, Teodor Sigaev wrote:
О©╫О©╫О©╫О©╫ Does the attached fix look correct?О©╫ Haven't checked the fix with
ATTACH
О©╫О©╫О©╫О©╫ PARTITION though.Attached patch seems to fix the problem.О©╫ However, I would rather get
rid of modifyingО©╫stmt->indexParams.О©╫ That seems to be more logical
for me.О©╫ Also, it would be good to check some covering indexes on
partitioned tables.О©╫ See the attached patch.Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.But it willО©╫ be better to have a ATTACH PARTITION test too.
I have added tests.О©╫ Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.Attached find updated patch.
Thanks,
Amit
--
Teodor Sigaev E-mail: teodor@sigaev.ru
WWW: http://www.sigaev.ru/
On Thu, Apr 12, 2018 at 6:14 AM, Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
I'm OK with collation of included columns to be the same as collation
of underlying table columns. But I still think we should throw an error
when user is trying to specify his own collation of included columns.
I agree. The collation of a table column is just setting a default
for how it gets interpreted in queries, but the collation of an index
column affects the ordering of the index. For INCLUDE columns, the
latter isn't relevant, so the value has no meaning. Letting people
set a meaningless value sometimes gets us into trouble (see also the
nearby thread on TABLESPACE settings on partitioned tables).
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company