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 712b0b0eba80b097103ac96167e7fc841e54fd9f..1bf1de56f315c40aa0901d4af49c30e1aa25e783 100644 (file)
--- 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 e99254950426b6edea30ae125b1f5ba8e538b79e..41bfa857ddd84e24017b1e682a8108e2384f882d 100644 (file)
--- 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 d9bf08d6d5067ae2fd1321c103f52214a23e082f..494c1504d7883329c7e4752636ec9f51dff6a507 100644 (file)
--- 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
 --