Prevent incorrect updates of pg_index while reindexing pg_index itself.

The places that attempt to change pg_index.indcheckxmin during a reindexing
operation cannot be executed safely if pg_index itself is the subject of
the operation.  This is the explanation for a couple of recent reports of
VACUUM FULL failing with
	ERROR:  duplicate key value violates unique constraint "pg_index_indexrelid_index"
	DETAIL:  Key (indexrelid)=(2678) already exists.

However, there isn't any real need to update indcheckxmin in such a
situation, if we assume that pg_index can never contain a truly broken HOT
chain.  This assumption holds if new indexes are never created on it during
concurrent operations, which is something we don't consider safe for any
system catalog, not just pg_index.  Accordingly, modify the code to not
manipulate indcheckxmin when reindexing any system catalog.

Back-patch to 8.3, where HOT was introduced.  The known failure scenarios
involve 9.0-style VACUUM FULL, so there might not be any real risk before
9.0, but let's not assume that.

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 487dbbf24d247251d3078839009687f7c96af7bc..79b96a07d8d052638c586258ae6c88ffde6c54b0 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1423,6 +1423,13 @@ index_build(Relation heapRelation,
        HeapTuple   indexTuple;
        Form_pg_index indexForm;
 
+       /*
+        * Broken HOT chains should not get reported in system catalogs; in
+        * particular it would be quite dangerous to try to modify the index's
+        * pg_index entry if we are reindexing pg_index itself.
+        */
+       Assert(!IsSystemRelation(heapRelation));
+
        pg_index = heap_open(IndexRelationId, RowExclusiveLock);
 
        indexTuple = SearchSysCacheCopy(INDEXRELID,
@@ -1482,7 +1489,13 @@ index_build(Relation heapRelation,
  * A side effect is to set indexInfo->ii_BrokenHotChain to true if we detect
  * any potentially broken HOT chains.  Currently, we set this if there are
  * any RECENTLY_DEAD entries in a HOT chain, without trying very hard to
- * detect whether they're really incompatible with the chain tip.
+ * detect whether they're really incompatible with the chain tip.  However,
+ * we do not ever set ii_BrokenHotChain true when the relation is a system
+ * catalog.  This is to avoid problematic behavior when reindexing pg_index
+ * itself: we can't safely change the index's indcheckxmin field when we're
+ * partway through such an operation.  It should be okay since the set of
+ * indexes on a system catalog ought not change during concurrent operations,
+ * so that no HOT chain in it could ever become broken.
  */
 double
 IndexBuildHeapScan(Relation heapRelation,
@@ -1492,6 +1505,7 @@ IndexBuildHeapScan(Relation heapRelation,
                   IndexBuildCallback callback,
                   void *callback_state)
 {
+   bool        is_system_catalog;
    HeapScanDesc scan;
    HeapTuple   heapTuple;
    Datum       values[INDEX_MAX_KEYS];
@@ -1511,6 +1525,9 @@ IndexBuildHeapScan(Relation heapRelation,
     */
    Assert(OidIsValid(indexRelation->rd_rel->relam));
 
+   /* Remember if it's a system catalog */
+   is_system_catalog = IsSystemRelation(heapRelation);
+
    /*
     * Need an EState for evaluation of index expressions and partial-index
     * predicates.  Also a slot to hold the current tuple.
@@ -1649,7 +1666,8 @@ IndexBuildHeapScan(Relation heapRelation,
                    {
                        indexIt = false;
                        /* mark the index as unsafe for old snapshots */
-                       indexInfo->ii_BrokenHotChain = true;
+                       if (!is_system_catalog)
+                           indexInfo->ii_BrokenHotChain = true;
                    }
                    else if (indexInfo->ii_BrokenHotChain)
                        indexIt = false;
@@ -1738,7 +1756,8 @@ IndexBuildHeapScan(Relation heapRelation,
                    {
                        indexIt = false;
                        /* mark the index as unsafe for old snapshots */
-                       indexInfo->ii_BrokenHotChain = true;
+                       if (!is_system_catalog)
+                           indexInfo->ii_BrokenHotChain = true;
                    }
                    else if (indexInfo->ii_BrokenHotChain)
                        indexIt = false;
@@ -2352,27 +2371,35 @@ reindex_index(Oid indexId)
     * 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.
+    *
+    * When reindexing a system catalog, don't do any of this --- it would be
+    * particularly risky to try to modify pg_index while we are reindexing
+    * pg_index itself.  We don't support CREATE INDEX CONCURRENTLY on system
+    * catalogs anyway, and they should never have indcheckxmin set either.
     */
-   pg_index = heap_open(IndexRelationId, RowExclusiveLock);
+   if (!IsSystemRelation(heapRelation))
+   {
+       pg_index = heap_open(IndexRelationId, RowExclusiveLock);
 
-   indexTuple = SearchSysCacheCopy(INDEXRELID,
-                                   ObjectIdGetDatum(indexId),
-                                   0, 0, 0);
-   if (!HeapTupleIsValid(indexTuple))
-       elog(ERROR, "cache lookup failed for index %u", indexId);
-   indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+       indexTuple = SearchSysCacheCopy(INDEXRELID,
+                                       ObjectIdGetDatum(indexId),
+                                       0, 0, 0);
+       if (!HeapTupleIsValid(indexTuple))
+           elog(ERROR, "cache lookup failed for index %u", indexId);
+       indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
 
-   if (!indexForm->indisvalid || !indexForm->indisready ||
-       (indexForm->indcheckxmin && !indexInfo->ii_BrokenHotChain))
-   {
-       indexForm->indisvalid = true;
-       indexForm->indisready = true;
-       if (!indexInfo->ii_BrokenHotChain)
-           indexForm->indcheckxmin = false;
-       simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
-       CatalogUpdateIndexes(pg_index, indexTuple);
+       if (!indexForm->indisvalid || !indexForm->indisready ||
+           (indexForm->indcheckxmin && !indexInfo->ii_BrokenHotChain))
+       {
+           indexForm->indisvalid = true;
+           indexForm->indisready = true;
+           if (!indexInfo->ii_BrokenHotChain)
+               indexForm->indcheckxmin = false;
+           simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
+           CatalogUpdateIndexes(pg_index, indexTuple);
+       }
+       heap_close(pg_index, RowExclusiveLock);
    }
-   heap_close(pg_index, RowExclusiveLock);
 
    /* Close rels, but keep locks */
    index_close(iRel, NoLock);