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.

diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 528ea23d4c3fcd023dfc11f98149f01b7e4a80a7..fc093ccc4b9aa339f0fa093e1d6d24cb40ff2699 100644 (file)
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -1134,7 +1134,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 09b26a5c724066e6c2c39f686953b771e22f7741..8c5670fdb8c49a041a7235a4f1e899aa4d865318 100644 (file)
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2569,7 +2569,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 9c96509600dbd97a35d2c582c3c887d4ddb85a31..2ccb9961122b929e88321b95151c4a97964ecf7c 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1061,7 +1061,7 @@ index_create(Relation heapRelation,
        }
        else
        {
-               index_build(heapRelation, indexRelation, indexInfo, isprimary);
+               index_build(heapRelation, indexRelation, indexInfo, isprimary, false);
        }
 
        /*
@@ -1680,8 +1680,11 @@ index_update_stats(Relation rel,
  * 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.
@@ -1691,7 +1694,8 @@ void
 index_build(Relation heapRelation,
                        Relation indexRelation,
                        IndexInfo *indexInfo,
-                       bool isprimary)
+                       bool isprimary,
+                       bool isreindex)
 {
        RegProcedure procedure;
        IndexBuildResult *stats;
@@ -1762,8 +1766,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;
@@ -1778,6 +1789,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);
@@ -2767,7 +2781,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
 
                /* 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();
        {
@@ -2787,6 +2801,16 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
         * We can also reset indcheckxmin, because we have now done a
         * non-concurrent index build, *except* in the case where index_build
         * found some still-broken HOT chains.
+        *
+        * 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.
         */
        if (!skipped_constraint)
        {

diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 10ec32c05de297824c32bad522ccbbbdcfcdac94..ff228b7d5310b958dd16e4d173a7fc6712280eaf 100644 (file)
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1398,6 +1398,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
         * advantage to the other order anyway because this is all transactional,
         * so no chance to reclaim disk space before commit.  We do not need a
         * final 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_flags = REINDEX_REL_SUPPRESS_INDEX_USE;
        if (check_constraints)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2f4e751100d3ca2ccfc71042030a5e9f8b89418b..53a6aafbbfb996e65ed5393e60d8533bfb9e8c7a 100644 (file)
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -505,7 +505,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 cc7e785cc0aa5ae1903783b33c254d3c2808f429..071db7f40128fdc20156587dbd64e07b53a33f6e 100644 (file)
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -75,7 +75,8 @@ extern void FormIndexDatum(IndexInfo *indexInfo,
 extern void index_build(Relation heapRelation,
                        Relation indexRelation,
                        IndexInfo *indexInfo,
-                       bool isprimary);
+                       bool isprimary,
+                       bool isreindex);
 
 extern double IndexBuildHeapScan(Relation heapRelation,
                                   Relation indexRelation,