Peter Geoghegan wrote:
On Wed, Apr 11, 2018 at 2:29 PM, Peter Eisentraut
<peter.eisentr...@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: teo...@sigaev.ru
WWW: http://www.sigaev.ru/
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
