From c266ed31a8a3beed3533e6a78faeca78234cbd43 Mon Sep 17 00:00:00 2001 From: Teodor Sigaev Date: Thu, 12 Apr 2018 16:37:22 +0300 Subject: [PATCH] Cleanup covering infrastructure - Explicitly forbids opclass, collation and indoptions (like DESC/ASC etc) for including columns. Throw an error if user points that. - Truncated storage arrays for such attributes to store only key atrributes, added assertion checks. - Do not check opfamily and collation for including columns in CompareIndexInfo() Discussion: https://www.postgresql.org/message-id/5ee72852-3c4e-ee35-e2ed-c1d053d45c08@sigaev.ru --- src/backend/catalog/index.c | 22 +++++++++++--- src/backend/commands/indexcmds.c | 44 +++++++++++++++++++-------- src/backend/optimizer/path/indxpath.c | 40 ++++++++++++++++++------ src/backend/optimizer/util/plancat.c | 4 +-- src/backend/parser/parse_utilcmd.c | 3 -- src/backend/utils/adt/ruleutils.c | 6 ++-- src/backend/utils/adt/selfuncs.c | 2 ++ src/backend/utils/cache/relcache.c | 8 ++--- 8 files changed, 90 insertions(+), 39 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 7fc7c01d76..dec4265d68 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_IndexAttrNumbers[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 @@ -1081,7 +1084,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) @@ -1832,6 +1835,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 @@ -1849,6 +1857,10 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2, info1->ii_IndexAttrNumbers[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 3d90204d42..61a4b24437 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..07d55a59ad 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -2329,8 +2329,8 @@ match_clause_to_indexcol(IndexOptInfo *index, { Expr *clause = rinfo->clause; Index index_relid = index->rel->relid; - Oid opfamily = index->opfamily[indexcol]; - Oid idxcollation = index->indexcollations[indexcol]; + Oid opfamily; + Oid idxcollation; Node *leftop, *rightop; Relids left_relids; @@ -2339,6 +2339,11 @@ match_clause_to_indexcol(IndexOptInfo *index, Oid expr_coll; bool plain_op; + Assert(indexcol < index->nkeycolumns); + + opfamily = index->opfamily[indexcol]; + idxcollation = index->indexcollations[indexcol]; + /* First check for boolean-index cases. */ if (IsBooleanOpfamily(opfamily)) { @@ -2678,8 +2683,8 @@ match_clause_to_ordering_op(IndexOptInfo *index, Expr *clause, Oid pk_opfamily) { - Oid opfamily = index->opfamily[indexcol]; - Oid idxcollation = index->indexcollations[indexcol]; + Oid opfamily; + Oid idxcollation; Node *leftop, *rightop; Oid expr_op; @@ -2687,6 +2692,10 @@ match_clause_to_ordering_op(IndexOptInfo *index, Oid sortfamily; bool commuted; + Assert(indexcol < index->nkeycolumns); + + opfamily = index->opfamily[indexcol]; + idxcollation = index->indexcollations[indexcol]; /* * Clause must be a binary opclause. */ @@ -2921,8 +2930,13 @@ ec_member_matches_indexcol(PlannerInfo *root, RelOptInfo *rel, { IndexOptInfo *index = ((ec_member_matches_arg *) arg)->index; int indexcol = ((ec_member_matches_arg *) arg)->indexcol; - Oid curFamily = index->opfamily[indexcol]; - Oid curCollation = index->indexcollations[indexcol]; + Oid curFamily; + Oid curCollation; + + Assert(indexcol < index->nkeycolumns); + + curFamily = index->opfamily[indexcol]; + curCollation = index->indexcollations[indexcol]; /* * If it's a btree index, we can reject it if its opfamily isn't @@ -3548,8 +3562,13 @@ expand_indexqual_conditions(IndexOptInfo *index, RestrictInfo *rinfo = (RestrictInfo *) lfirst(lcc); int indexcol = lfirst_int(lci); Expr *clause = rinfo->clause; - Oid curFamily = index->opfamily[indexcol]; - Oid curCollation = index->indexcollations[indexcol]; + Oid curFamily; + Oid curCollation; + + Assert(indexcol < index->nkeycolumns); + + curFamily = index->opfamily[indexcol]; + curCollation = index->indexcollations[indexcol]; /* First check for boolean cases */ if (IsBooleanOpfamily(curFamily)) @@ -3918,14 +3937,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 04ee5f2015..cb9cfc3729 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 fd2a3cc8c3..4939d1e348 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 -- 2.40.0