From 8ceb24568054232696dddc1166a8563bc78c900a Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Thu, 20 Jan 2011 22:44:10 -0500
Subject: [PATCH] Make ALTER TABLE revalidate uniqueness and exclusion
constraints.
Failure to do so can lead to constraint violations. This was broken by
commit 1ddc2703a936d03953657f43345460b9242bbed1 on 2010-02-07, so
back-patch to 9.0.
Noah Misch. Regression test by me.
---
src/backend/catalog/index.c | 43 +++++++++++++----------
src/backend/commands/cluster.c | 9 +++--
src/backend/commands/indexcmds.c | 4 +--
src/backend/commands/tablecmds.c | 7 ++--
src/include/catalog/index.h | 5 ++-
src/include/commands/cluster.h | 1 +
src/test/regress/expected/alter_table.out | 4 +++
src/test/regress/sql/alter_table.sql | 2 ++
8 files changed, 48 insertions(+), 27 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5790f872fe6..7b64a7af446 100644
--- 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
index 19c3cf9674c..59a439413e1 100644
--- 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
index f809a26369e..7a6a4c33adc 100644
--- 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
index f3bd565b986..a9bb8351bc1 100644
--- 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
index 2804c6338ea..29c471703c2 100644
--- 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
index 427d802a0b3..518e8969c9d 100644
--- 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
index 3d126bbf308..3280065f75b 100644
--- 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
index 4895768d7fe..cfbfbb6b438 100644
--- 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
--
GitLab