Make ALTER TABLE revalidate uniqueness and exclusion constraints.

robertmhaas · robertmhaas · commit 8ceb24568054 · 2011-01-20T22:44:10.000-05:00
Failure to do so can lead to constraint violations. This was broken by commit 1ddc270 on 2010-02-07, so back-patch to 9.0. Noah Misch. Regression test by me.
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
@@ -2529,26 +2529,29 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
  * reindex_relation - This routine is used to recreate all indexes
  * of a relation (and optionally its toast relation too, if any).
  *
- * If heap_rebuilt is true, then the relation was just completely rebuilt by
- * an operation such as VACUUM FULL or CLUSTER, and therefore its indexes are
- * inconsistent with it.  This makes things tricky if the relation is a system
- * catalog that we might consult during the reindexing.  To deal with that
- * case, we mark all of the indexes as pending rebuild so that they won't be
- * trusted until rebuilt.  The caller is required to call us *without* having
- * made the rebuilt versions visible by doing CommandCounterIncrement; we'll
- * do CCI after having collected the index list.  (This way we can still use
- * catalog indexes while collecting the list.)
+ * "flags" can include REINDEX_SUPPRESS_INDEX_USE and REINDEX_CHECK_CONSTRAINTS.
  *
- * We also skip rechecking uniqueness/exclusion constraint properties if
- * heap_rebuilt is true.  This avoids likely deadlock conditions when doing
- * VACUUM FULL or CLUSTER on system catalogs.  REINDEX should be used to
- * rebuild an index if constraint inconsistency is suspected.
+ * If flags has REINDEX_SUPPRESS_INDEX_USE, the relation was just completely
+ * rebuilt by an operation such as VACUUM FULL or CLUSTER, and therefore its
+ * indexes are inconsistent with it.  This makes things tricky if the relation
+ * is a system catalog that we might consult during the reindexing.  To deal
+ * with that case, we mark all of the indexes as pending rebuild so that they
+ * won't be trusted until rebuilt.  The caller is required to call us *without*
+ * having made the rebuilt versions visible by doing CommandCounterIncrement;
+ * we'll do CCI after having collected the index list.  (This way we can still
+ * use catalog indexes while collecting the list.)
+ *
+ * To avoid deadlocks, VACUUM FULL or CLUSTER on a system catalog must omit the
+ * REINDEX_CHECK_CONSTRAINTS flag.  REINDEX should be used to rebuild an index
+ * if constraint inconsistency is suspected.  For optimal performance, other
+ * callers should include the flag only after transforming the data in a manner
+ * that risks a change in constraint validity.
  *
  * Returns true if any indexes were rebuilt.  Note that a
  * CommandCounterIncrement will occur after each index rebuild.
  */
 bool
-reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
+reindex_relation(Oid relid, bool toast_too, int flags)
 {
 	Relation	rel;
 	Oid			toast_relid;
@@ -2604,7 +2607,7 @@ reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
 		List	   *doneIndexes;
 		ListCell   *indexId;
 
-		if (heap_rebuilt)
+		if (flags & REINDEX_SUPPRESS_INDEX_USE)
 		{
 			/* Suppress use of all the indexes until they are rebuilt */
 			SetReindexPending(indexIds);
@@ -2625,11 +2628,11 @@ reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
 			if (is_pg_class)
 				RelationSetIndexList(rel, doneIndexes, InvalidOid);
 
-			reindex_index(indexOid, heap_rebuilt);
+			reindex_index(indexOid, !(flags & REINDEX_CHECK_CONSTRAINTS));
 
 			CommandCounterIncrement();
 
-			if (heap_rebuilt)
+			if (flags & REINDEX_SUPPRESS_INDEX_USE)
 				RemoveReindexPending(indexOid);
 
 			if (is_pg_class)
@@ -2657,10 +2660,12 @@ reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt)
 
 	/*
 	 * If the relation has a secondary toast rel, reindex that too while we
-	 * still hold the lock on the master table.
+	 * still hold the lock on the master table.  There's never a reason to
+	 * reindex the toast table right after rebuilding the heap.
 	 */
+	Assert(!(toast_too && (flags & REINDEX_SUPPRESS_INDEX_USE)));
 	if (toast_too && OidIsValid(toast_relid))
-		result |= reindex_relation(toast_relid, false, false);
+		result |= reindex_relation(toast_relid, false, flags);
 
 	return result;
 }
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
@@ -565,7 +565,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid,
 	 * rebuild the target's indexes and throw away the transient table.
 	 */
 	finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
-					 swap_toast_by_content, frozenXid);
+					 swap_toast_by_content, false, frozenXid);
 }
 
 
@@ -1362,10 +1362,12 @@ void
 finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 				 bool is_system_catalog,
 				 bool swap_toast_by_content,
+				 bool check_constraints,
 				 TransactionId frozenXid)
 {
 	ObjectAddress object;
 	Oid			mapped_tables[4];
+	int			reindex_flags;
 	int			i;
 
 	/* Zero out possible results from swapped_relation_files */
@@ -1395,7 +1397,10 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 	 * so no chance to reclaim disk space before commit.  We do not need a
 	 * final CommandCounterIncrement() because reindex_relation does it.
 	 */
-	reindex_relation(OIDOldHeap, false, true);
+	reindex_flags = REINDEX_SUPPRESS_INDEX_USE;
+	if (check_constraints)
+		reindex_flags |= REINDEX_CHECK_CONSTRAINTS;
+	reindex_relation(OIDOldHeap, false, reindex_flags);
 
 	/* Destroy new heap with old filenode */
 	object.classId = RelationRelationId;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
@@ -1629,7 +1629,7 @@ ReindexTable(RangeVar *relation)
 
 	ReleaseSysCache(tuple);
 
-	if (!reindex_relation(heapOid, true, false))
+	if (!reindex_relation(heapOid, true, 0))
 		ereport(NOTICE,
 				(errmsg("table \"%s\" has no indexes",
 						relation->relname)));
@@ -1742,7 +1742,7 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user)
 		StartTransactionCommand();
 		/* functions in indexes may want a snapshot set */
 		PushActiveSnapshot(GetTransactionSnapshot());
-		if (reindex_relation(relid, true, false))
+		if (reindex_relation(relid, true, 0))
 			ereport(NOTICE,
 					(errmsg("table \"%s.%s\" was reindexed",
 							get_namespace_name(get_rel_namespace(relid)),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
@@ -1076,7 +1076,7 @@ ExecuteTruncate(TruncateStmt *stmt)
 			/*
 			 * Reconstruct the indexes to match, and we're done.
 			 */
-			reindex_relation(heap_relid, true, false);
+			reindex_relation(heap_relid, true, 0);
 		}
 	}
 
@@ -3236,13 +3236,14 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 
 			/*
 			 * Swap the physical files of the old and new heaps, then rebuild
-			 * indexes and discard the new heap.  We can use RecentXmin for
+			 * indexes and discard the old heap.  We can use RecentXmin for
 			 * the table's new relfrozenxid because we rewrote all the tuples
 			 * in ATRewriteTable, so no older Xid remains in the table.  Also,
 			 * we never try to swap toast tables by content, since we have no
 			 * interest in letting this code work on system catalogs.
 			 */
-			finish_heap_swap(tab->relid, OIDNewHeap, false, false, RecentXmin);
+			finish_heap_swap(tab->relid, OIDNewHeap,
+							 false, false, true, RecentXmin);
 		}
 		else
 		{
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
@@ -71,7 +71,10 @@ extern double IndexBuildHeapScan(Relation heapRelation,
 extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
 
 extern void reindex_index(Oid indexId, bool skip_constraint_checks);
-extern bool reindex_relation(Oid relid, bool toast_too, bool heap_rebuilt);
+
+#define REINDEX_CHECK_CONSTRAINTS	0x1
+#define REINDEX_SUPPRESS_INDEX_USE	0x2
+extern bool reindex_relation(Oid relid, bool toast_too, int flags);
 
 extern bool ReindexIsProcessingHeap(Oid heapOid);
 extern bool ReindexIsProcessingIndex(Oid indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
@@ -29,6 +29,7 @@ extern Oid	make_new_heap(Oid OIDOldHeap, Oid NewTableSpace);
 extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 				 bool is_system_catalog,
 				 bool swap_toast_by_content,
+				 bool check_constraints,
 				 TransactionId frozenXid);
 
 #endif   /* CLUSTER_H */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
@@ -413,6 +413,10 @@ insert into atacc1 (test) values (4);
 -- try adding a unique oid constraint
 alter table atacc1 add constraint atacc_oid1 unique(oid);
 NOTICE:  ALTER TABLE / ADD UNIQUE will create implicit index "atacc_oid1" for table "atacc1"
+-- try to create duplicates via alter table using - should fail
+alter table atacc1 alter column test type integer using 0;
+ERROR:  could not create unique index "atacc_test1"
+DETAIL:  Key (test)=(0) is duplicated.
 drop table atacc1;
 -- let's do one where the unique constraint fails when added
 create table atacc1 ( test int );
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
@@ -419,6 +419,8 @@ insert into atacc1 (test) values (2);
 insert into atacc1 (test) values (4);
 -- try adding a unique oid constraint
 alter table atacc1 add constraint atacc_oid1 unique(oid);
+-- try to create duplicates via alter table using - should fail
+alter table atacc1 alter column test type integer using 0;
 drop table atacc1;
 
 -- let's do one where the unique constraint fails when added

Original file line number	Diff line number	Diff line change
`@@ -1076,7 +1076,7 @@ ExecuteTruncate(TruncateStmt *stmt)`
`1076`	`1076`	`/*`
`1077`	`1077`	`* Reconstruct the indexes to match, and we're done.`
`1078`	`1078`	`*/`
`1079`		`- reindex_relation(heap_relid, true, false);`
	`1079`	`+ reindex_relation(heap_relid, true, 0);`
`1080`	`1080`	`}`
`1081`	`1081`	`}`
`1082`	`1082`
`@@ -3236,13 +3236,14 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)`
`3236`	`3236`
`3237`	`3237`	`/*`
`3238`	`3238`	`* Swap the physical files of the old and new heaps, then rebuild`
`3239`		`- * indexes and discard the new heap. We can use RecentXmin for`
	`3239`	`+ * indexes and discard the old heap. We can use RecentXmin for`
`3240`	`3240`	`* the table's new relfrozenxid because we rewrote all the tuples`
`3241`	`3241`	`* in ATRewriteTable, so no older Xid remains in the table. Also,`
`3242`	`3242`	`* we never try to swap toast tables by content, since we have no`
`3243`	`3243`	`* interest in letting this code work on system catalogs.`
`3244`	`3244`	`*/`
`3245`		`- finish_heap_swap(tab->relid, OIDNewHeap, false, false, RecentXmin);`
	`3245`	`+ finish_heap_swap(tab->relid, OIDNewHeap,`
	`3246`	`+ false, false, true, RecentXmin);`
`3246`	`3247`	`}`
`3247`	`3248`	`else`
`3248`	`3249`	`{`