From: Robert Haas Date: Wed, 25 Jan 2012 20:28:07 +0000 (-0500) Subject: Make CheckIndexCompatible simpler and more bullet-proof. X-Git-Tag: REL9_2_BETA1~544 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6eb71ac5527a94be443bc66e68b47b04979906e4;p=postgresql Make CheckIndexCompatible simpler and more bullet-proof. This gives up the "don't rewrite the index" behavior in a couple of relatively unimportant cases, such as changing between an array type and an unconstrained domain over that array type, in return for making this code more future-proof. Noah Misch --- diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 712b0b0eba..1bf1de56f3 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -23,6 +23,7 @@ #include "catalog/pg_opclass.h" #include "catalog/pg_opfamily.h" #include "catalog/pg_tablespace.h" +#include "catalog/pg_type.h" #include "commands/dbcommands.h" #include "commands/defrem.h" #include "commands/tablecmds.h" @@ -52,6 +53,7 @@ /* non-export function prototypes */ static void CheckPredicate(Expr *predicate); static void ComputeIndexAttrs(IndexInfo *indexInfo, + Oid *typeOidP, Oid *collationOidP, Oid *classOidP, int16 *colOptionP, @@ -87,18 +89,17 @@ static void RangeVarCallbackForReindexIndex(const RangeVar *relation, * of columns and that if one has an expression column or predicate, both do. * Errors arising from the attribute list still apply. * - * Most column type changes that can skip a table rewrite will not invalidate - * indexes. For btree and hash indexes, we assume continued validity when - * each column of an index would have the same operator family before and - * after the change. Since we do not document a contract for GIN or GiST - * operator families, we require an exact operator class match for them and - * for any other access methods. - * - * DefineIndex always verifies that each exclusion operator shares an operator - * family with its corresponding index operator class. For access methods - * having no operator family contract, confirm that the old and new indexes - * use the exact same exclusion operator. For btree and hash, there's nothing - * more to check. + * Most column type changes that can skip a table rewrite do not invalidate + * indexes. We ackowledge this when all operator classes, collations and + * exclusion operators match. Though we could further permit intra-opfamily + * changes for btree and hash indexes, that adds subtle complexity with no + * concrete benefit for core types. + + * When a comparison or exclusion operator has a polymorphic input type, the + * actual input types must also match. This defends against the possibility + * that operators could vary behavior in response to get_fn_expr_argtype(). + * At present, this hazard is theoretical: check_exclusion_constraint() and + * all core index access methods decline to set fn_expr for such calls. * * We do not yet implement a test to verify compatibility of expression * columns or predicates, so assume any such index is incompatible. @@ -111,6 +112,7 @@ CheckIndexCompatible(Oid oldId, List *exclusionOpNames) { bool isconstraint; + Oid *typeObjectId; Oid *collationObjectId; Oid *classObjectId; Oid accessMethodId; @@ -123,10 +125,10 @@ CheckIndexCompatible(Oid oldId, int numberOfAttributes; int old_natts; bool isnull; - bool family_am; bool ret = true; oidvector *old_indclass; oidvector *old_indcollation; + Relation irel; int i; Datum d; @@ -168,10 +170,12 @@ CheckIndexCompatible(Oid oldId, indexInfo->ii_ExclusionOps = NULL; indexInfo->ii_ExclusionProcs = NULL; indexInfo->ii_ExclusionStrats = NULL; + typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16)); - ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId, + ComputeIndexAttrs(indexInfo, + typeObjectId, collationObjectId, classObjectId, coloptions, attributeList, exclusionOpNames, relationId, accessMethodName, accessMethodId, @@ -191,12 +195,7 @@ CheckIndexCompatible(Oid oldId, return false; } - /* - * If the old and new operator class of any index column differ in - * operator family or collation, regard the old index as incompatible. - * For access methods other than btree and hash, a family match has no - * defined meaning; require an exact operator class match. - */ + /* Any change in operator class or collation breaks compatibility. */ old_natts = ((Form_pg_index) GETSTRUCT(tuple))->indnatts; Assert(old_natts == numberOfAttributes); @@ -208,52 +207,42 @@ CheckIndexCompatible(Oid oldId, Assert(!isnull); old_indclass = (oidvector *) DatumGetPointer(d); - family_am = accessMethodId == BTREE_AM_OID || accessMethodId == HASH_AM_OID; - - for (i = 0; i < old_natts; i++) - { - Oid old_class = old_indclass->values[i]; - Oid new_class = classObjectId[i]; - - if (!(old_indcollation->values[i] == collationObjectId[i] - && (old_class == new_class - || (family_am && (get_opclass_family(old_class) - == get_opclass_family(new_class)))))) - { - ret = false; - break; - } - } + ret = (memcmp(old_indclass->values, classObjectId, + old_natts * sizeof(Oid)) == 0 && + memcmp(old_indcollation->values, collationObjectId, + old_natts * sizeof(Oid)) == 0); ReleaseSysCache(tuple); - /* - * For btree and hash, exclusion operators need only fall in the same - * operator family; ComputeIndexAttrs already verified that much. If we - * get this far, we know that the index operator family has not changed, - * and we're done. For other access methods, require exact matches for - * all exclusion operators. - */ - if (ret && !family_am && indexInfo->ii_ExclusionOps != NULL) + /* For polymorphic opcintype, column type changes break compatibility. */ + irel = index_open(oldId, AccessShareLock); /* caller probably has a lock */ + for (i = 0; i < old_natts && ret; i++) + ret = (!IsPolymorphicType(get_opclass_input_type(classObjectId[i])) || + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]); + + /* Any change in exclusion operator selections breaks compatibility. */ + if (ret && indexInfo->ii_ExclusionOps != NULL) { - Relation irel; Oid *old_operators, *old_procs; uint16 *old_strats; - /* Caller probably already holds a stronger lock. */ - irel = index_open(oldId, AccessShareLock); RelationGetExclusionInfo(irel, &old_operators, &old_procs, &old_strats); + ret = memcmp(old_operators, indexInfo->ii_ExclusionOps, + old_natts * sizeof(Oid)) == 0; - for (i = 0; i < old_natts; i++) - if (old_operators[i] != indexInfo->ii_ExclusionOps[i]) - { - ret = false; - break; - } + /* Require an exact input type match for polymorphic operators. */ + for (i = 0; i < old_natts && ret; i++) + { + Oid left, + right; - index_close(irel, NoLock); + op_input_types(indexInfo->ii_ExclusionOps[i], &left, &right); + ret = (!(IsPolymorphicType(left) || IsPolymorphicType(right)) || + irel->rd_att->attrs[i]->atttypid == typeObjectId[i]); + } } + index_close(irel, NoLock); return ret; } @@ -315,6 +304,7 @@ DefineIndex(RangeVar *heapRelation, bool quiet, bool concurrent) { + Oid *typeObjectId; Oid *collationObjectId; Oid *classObjectId; Oid accessMethodId; @@ -550,10 +540,12 @@ DefineIndex(RangeVar *heapRelation, indexInfo->ii_Concurrent = concurrent; indexInfo->ii_BrokenHotChain = false; + typeObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); collationObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); classObjectId = (Oid *) palloc(numberOfAttributes * sizeof(Oid)); coloptions = (int16 *) palloc(numberOfAttributes * sizeof(int16)); - ComputeIndexAttrs(indexInfo, collationObjectId, classObjectId, + ComputeIndexAttrs(indexInfo, + typeObjectId, collationObjectId, classObjectId, coloptions, attributeList, exclusionOpNames, relationId, accessMethodName, accessMethodId, @@ -980,6 +972,7 @@ CheckPredicate(Expr *predicate) */ static void ComputeIndexAttrs(IndexInfo *indexInfo, + Oid *typeOidP, Oid *collationOidP, Oid *classOidP, int16 *colOptionP, @@ -1108,6 +1101,8 @@ ComputeIndexAttrs(IndexInfo *indexInfo, } } + typeOidP[attn] = atttype; + /* * Apply collation override if any */ diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index e992549504..41bfa857dd 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -1665,6 +1665,15 @@ where oid = 'test_storage'::regclass; t (1 row) +-- SET DATA TYPE without a rewrite +CREATE DOMAIN other_textarr AS text[]; +CREATE TABLE norewrite_array(c text[] PRIMARY KEY); +NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "norewrite_array_pkey" for table "norewrite_array" +SET client_min_messages = debug1; +ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work +ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index +DEBUG: building index "norewrite_array_pkey" on table "norewrite_array" +RESET client_min_messages; -- -- lock levels -- diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index d9bf08d6d5..494c1504d7 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1197,6 +1197,14 @@ select reltoastrelid <> 0 as has_toast_table from pg_class where oid = 'test_storage'::regclass; +-- SET DATA TYPE without a rewrite +CREATE DOMAIN other_textarr AS text[]; +CREATE TABLE norewrite_array(c text[] PRIMARY KEY); +SET client_min_messages = debug1; +ALTER TABLE norewrite_array ALTER c TYPE text[]; -- no work +ALTER TABLE norewrite_array ALTER c TYPE other_textarr; -- rebuild index +RESET client_min_messages; + -- -- lock levels --