From 8cf0208b705aebbb23da7fc6a7a20772279bae26 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 19 Apr 2011 18:51:12 -0400 Subject: [PATCH] Avoid changing an index's indcheckxmin horizon during REINDEX. There can never be a need to push the indcheckxmin horizon forward, since any HOT chains that are actually broken with respect to the index must pre-date its original creation. So we can just avoid changing pg_index altogether during a REINDEX operation. This offers a cleaner solution than my previous patch for the problem found a few days ago that we mustn't try to update pg_index while we are reindexing it. System catalog indexes will always be created with indcheckxmin = false during initdb, and with this modified code we should never try to change their pg_index entries. This avoids special-casing system catalogs as the former patch did, and should provide a performance benefit for many cases where REINDEX formerly caused an index to be considered unusable for a short time. Back-patch to 8.3 to cover all versions containing HOT. Note that this patch changes the API for index_build(), but I believe it is unlikely that any add-on code is calling that directly. --- src/backend/bootstrap/bootstrap.c | 2 +- src/backend/catalog/heap.c | 2 +- src/backend/catalog/index.c | 38 +++++++++++++++++++++++++------ src/backend/commands/cluster.c | 6 +++++ src/backend/commands/indexcmds.c | 2 +- src/include/catalog/index.h | 3 ++- 6 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 5450313be9c..6183b475c59 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -1285,7 +1285,7 @@ build_indices(void) heap = heap_open(ILHead->il_heap, NoLock); ind = index_open(ILHead->il_ind, NoLock); - index_build(heap, ind, ILHead->il_info, false); + index_build(heap, ind, ILHead->il_info, false, false); index_close(ind, NoLock); heap_close(heap, NoLock); diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index edc5427341a..aab4359edbd 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2196,7 +2196,7 @@ RelationTruncateIndexes(Relation heapRelation) /* Initialize the index and rebuild */ /* Note: we do not need to re-establish pkey setting */ - index_build(heapRelation, currentIndex, indexInfo, false); + index_build(heapRelation, currentIndex, indexInfo, false, true); /* We're done with this index */ index_close(currentIndex, NoLock); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 0f33de7b60d..4527e559e09 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -836,7 +836,7 @@ index_create(Oid heapRelationId, } else { - index_build(heapRelation, indexRelation, indexInfo, isprimary); + index_build(heapRelation, indexRelation, indexInfo, isprimary, false); } /* @@ -1328,8 +1328,11 @@ setNewRelfilenode(Relation relation, TransactionId freezeXid) * entries of the index and heap relation as needed, using statistics * returned by ambuild as well as data passed by the caller. * - * Note: when reindexing an existing index, isprimary can be false; - * the index is already properly marked and need not be re-marked. + * isprimary tells whether to mark the index as a primary-key index. + * isreindex indicates we are recreating a previously-existing index. + * + * Note: when reindexing an existing index, isprimary can be false even if + * the index is a PK; it's already properly marked and need not be re-marked. * * Note: before Postgres 8.2, the passed-in heap and index Relations * were automatically closed by this routine. This is no longer the case. @@ -1339,7 +1342,8 @@ void index_build(Relation heapRelation, Relation indexRelation, IndexInfo *indexInfo, - bool isprimary) + bool isprimary, + bool isreindex) { RegProcedure procedure; IndexBuildResult *stats; @@ -1386,8 +1390,15 @@ index_build(Relation heapRelation, * If we found any potentially broken HOT chains, mark the index as not * being usable until the current transaction is below the event horizon. * See src/backend/access/heap/README.HOT for discussion. - */ - if (indexInfo->ii_BrokenHotChain) + * + * However, when reindexing an existing index, we should do nothing here. + * Any HOT chains that are broken with respect to the index must predate + * the index's original creation, so there is no need to change the + * index's usability horizon. Moreover, we *must not* try to change + * the index's pg_index entry while reindexing pg_index itself, and this + * optimization nicely prevents that. + */ + if (indexInfo->ii_BrokenHotChain && !isreindex) { Oid indexId = RelationGetRelid(indexRelation); Relation pg_index; @@ -1403,6 +1414,9 @@ index_build(Relation heapRelation, elog(ERROR, "cache lookup failed for index %u", indexId); indexForm = (Form_pg_index) GETSTRUCT(indexTuple); + /* If it's a new index, indcheckxmin shouldn't be set ... */ + Assert(!indexForm->indcheckxmin); + indexForm->indcheckxmin = true; simple_heap_update(pg_index, &indexTuple->t_self, indexTuple); CatalogUpdateIndexes(pg_index, indexTuple); @@ -2297,7 +2311,7 @@ reindex_index(Oid indexId) /* Initialize the index and rebuild */ /* Note: we do not need to re-establish pkey setting */ - index_build(heapRelation, iRel, indexInfo, false); + index_build(heapRelation, iRel, indexInfo, false, true); } PG_CATCH(); { @@ -2312,6 +2326,16 @@ reindex_index(Oid indexId) * If the index is marked invalid or not ready (ie, it's from a failed * CREATE INDEX CONCURRENTLY), we can now mark it valid. This allows * REINDEX to be used to clean up in such cases. + * + * Note that it is important to not update the pg_index entry if we don't + * have to, because updating it will move the index's usability horizon + * (recorded as the tuple's xmin value) if indcheckxmin is true. We don't + * really want REINDEX to move the usability horizon forward ever, but we + * have no choice if we are to fix indisvalid or indisready. Of course, + * clearing indcheckxmin eliminates the issue, so we're happy to do that + * if we can. Another reason for caution here is that while reindexing + * pg_index itself, we must not try to update it. We assume that + * pg_index's indexes will always have these flags in their clean state. */ pg_index = heap_open(IndexRelationId, RowExclusiveLock); diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 7a9ad8d19a8..53f588eea8e 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -614,6 +614,12 @@ rebuild_relation(Relation OldHeap, Oid indexOid) * Rebuild each index on the relation (but not the toast table, which is * all-new at this point). We do not need CommandCounterIncrement() * because reindex_relation does it. + * + * Note: because index_build is called via reindex_relation, it will never + * set indcheckxmin true for the indexes. This is OK even though in some + * sense we are building new indexes rather than rebuilding existing ones, + * because the new heap won't contain any HOT chains at all, let alone + * broken ones, so it can't be necessary to set indcheckxmin. */ reindex_relation(tableOid, false); } diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index fe4a1ef1b05..6052eed7d9e 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -548,7 +548,7 @@ DefineIndex(RangeVar *heapRelation, indexInfo->ii_BrokenHotChain = false; /* Now build the index */ - index_build(rel, indexRelation, indexInfo, primary); + index_build(rel, indexRelation, indexInfo, primary, false); /* Close both the relations, but keep the locks */ heap_close(rel, NoLock); diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 70c265232bf..a92201546b1 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -58,7 +58,8 @@ extern void setNewRelfilenode(Relation relation, TransactionId freezeXid); extern void index_build(Relation heapRelation, Relation indexRelation, IndexInfo *indexInfo, - bool isprimary); + bool isprimary, + bool isreindex); extern double IndexBuildHeapScan(Relation heapRelation, Relation indexRelation, -- 2.39.5