Fix bugs in indexing of in-doubt HOT-updated tuples.

If we find a DELETE_IN_PROGRESS HOT-updated tuple, it is impossible to know
whether to index it or not except by waiting to see if the deleting
transaction commits.  If it doesn't, the tuple might again be LIVE, meaning
we have to index it.  So wait and recheck in that case.

Also, we must not rely on ii_BrokenHotChain to decide that it's possible to
omit tuples from the index.  That could result in omitting tuples that we
need, particularly in view of yesterday's fixes to not necessarily set
indcheckxmin (but it's broken even without that, as per my analysis today).
Since this is just an extremely marginal performance optimization, dropping
the test shouldn't hurt.

These cases are only expected to happen in system catalogs (they're
possible there due to early release of RowExclusiveLock in most
catalog-update code paths).  Since reindexing of a system catalog isn't a
particularly performance-critical operation anyway, there's no real need to
be concerned about possible performance degradation from these changes.

The worst aspects of this bug were introduced in 9.0 --- 8.x will always
wait out a DELETE_IN_PROGRESS tuple.  But I think dropping index entries
on the strength of ii_BrokenHotChain is dangerous even without that, so
back-patch removal of that optimization to 8.3 and 8.4.

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 4527e559e09b6378bdd069afbf2c3ab9ea998997..93eb2d1b495d35d159a7cb1b6752da8f1675802f 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1466,8 +1466,9 @@ 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.
+ * any RECENTLY_DEAD or DELETE_IN_PROGRESS entries in a HOT chain, without
+ * trying very hard to detect whether they're really incompatible with the
+ * chain tip.
  */
 double
 IndexBuildHeapScan(Relation heapRelation,
@@ -1570,8 +1571,14 @@ IndexBuildHeapScan(Relation heapRelation,
                 * buffer continuously while visiting the page, so no pruning
                 * operation can occur either.
                 *
+                * Also, although our opinions about tuple liveness could change while
+                * we scan the page (due to concurrent transaction commits/aborts),
+                * the chain root locations won't, so this info doesn't need to be
+                * rebuilt after waiting for another transaction.
+                *
                 * Note the implied assumption that there is no more than one live
-                * tuple per HOT-chain ...
+                * tuple per HOT-chain --- else we could create more than one index
+                * entry pointing to the same root tuple.
                 */
                if (scan->rs_cblock != root_blkno)
                {
@@ -1624,11 +1631,6 @@ IndexBuildHeapScan(Relation heapRelation,
                                         * the live tuple at the end of the HOT-chain.  Since this
                                         * breaks semantics for pre-existing snapshots, mark the
                                         * index as unusable for them.
-                                        *
-                                        * If we've already decided that the index will be unsafe
-                                        * for old snapshots, we may as well stop indexing
-                                        * recently-dead tuples, since there's no longer any
-                                        * point.
                                         */
                                        if (HeapTupleIsHotUpdated(heapTuple))
                                        {
@@ -1636,8 +1638,6 @@ IndexBuildHeapScan(Relation heapRelation,
                                                /* mark the index as unsafe for old snapshots */
                                                indexInfo->ii_BrokenHotChain = true;
                                        }
-                                       else if (indexInfo->ii_BrokenHotChain)
-                                               indexIt = false;
                                        else
                                                indexIt = true;
                                        /* In any case, exclude the tuple from unique-checking */
@@ -1685,16 +1685,8 @@ IndexBuildHeapScan(Relation heapRelation,
                                case HEAPTUPLE_DELETE_IN_PROGRESS:
 
                                        /*
-                                        * Since caller should hold ShareLock or better, we should
-                                        * not see any tuples deleted by open transactions ---
-                                        * unless it's our own transaction. (Consider DELETE
-                                        * followed by CREATE INDEX within a transaction.)      An
-                                        * exception occurs when reindexing a system catalog,
-                                        * because we often release lock on system catalogs before
-                                        * committing.  In that case we wait for the deleting
-                                        * transaction to finish and check again.  (We could do
-                                        * that on user tables too, but since the case is not
-                                        * expected it seems better to throw an error.)
+                                        * As with INSERT_IN_PROGRESS case, this is unexpected
+                                        * unless it's our own deletion or a system catalog.
                                         */
                                        Assert(!(heapTuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI));
                                        if (!TransactionIdIsCurrentTransactionId(
@@ -1713,22 +1705,35 @@ IndexBuildHeapScan(Relation heapRelation,
                                                        XactLockTableWait(xwait);
                                                        goto recheck;
                                                }
-                                       }
 
-                                       /*
-                                        * Otherwise, we have to treat these tuples just like
-                                        * RECENTLY_DELETED ones.
-                                        */
-                                       if (HeapTupleIsHotUpdated(heapTuple))
+                                               /*
+                                                * Otherwise index it but don't check for uniqueness,
+                                                * the same as a RECENTLY_DEAD tuple.  (We can't
+                                                * actually get here, but keep compiler quiet.)
+                                                */
+                                               indexIt = true;
+                                       }
+                                       else if (HeapTupleIsHotUpdated(heapTuple))
                                        {
+                                               /*
+                                                * It's a HOT-updated tuple deleted by our own xact.
+                                                * We can assume the deletion will commit (else the
+                                                * index contents don't matter), so treat the same
+                                                * as RECENTLY_DEAD HOT-updated tuples.
+                                                */
                                                indexIt = false;
                                                /* mark the index as unsafe for old snapshots */
                                                indexInfo->ii_BrokenHotChain = true;
                                        }
-                                       else if (indexInfo->ii_BrokenHotChain)
-                                               indexIt = false;
                                        else
+                                       {
+                                               /*
+                                                * It's a regular tuple deleted by our own xact.
+                                                * Index it but don't check for uniqueness, the same
+                                                * as a RECENTLY_DEAD tuple.
+                                                */
                                                indexIt = true;
+                                       }
                                        /* In any case, exclude the tuple from unique-checking */
                                        tupleIsAlive = false;
                                        break;