From 39b5e5f3370258cae843e8cc83eccd59ddb532dd Mon Sep 17 00:00:00 2001 From: Robert Haas 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 a3ac44cf79..8af382b9c5 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2508,26 +2508,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; @@ -2583,7 +2586,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); @@ -2604,11 +2607,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) @@ -2636,10 +2639,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 7a1b8e885b..61020dcbe7 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -607,7 +607,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); } @@ -1326,10 +1326,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 */ @@ -1359,7 +1361,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 780dbc23ed..a129511128 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1634,7 +1634,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))); @@ -1747,7 +1747,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 ab9c6a5191..6a1804b6fe 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -1032,7 +1032,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); } } @@ -2941,13 +2941,14 @@ ATRewriteTables(List **wqueue) /* * 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 6b5f3c43c8..18f17037b7 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 ed3853af24..a5aeb1aa10 100644 --- a/src/include/commands/cluster.h +++ b/src/include/commands/cluster.h @@ -28,6 +28,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 5aff44f23a..40a907d869 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 82c2e4ee01..2b01238e28 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 -- 2.11.0