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 6c81d57..c79402c 100644 (file)
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1840,8 +1840,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,
@@ -1953,8 +1954,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)
                {
@@ -2008,11 +2015,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))
                                        {
@@ -2020,8 +2022,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 */
@@ -2071,7 +2071,8 @@ IndexBuildHeapScan(Relation heapRelation,
                                case HEAPTUPLE_DELETE_IN_PROGRESS:
 
                                        /*
-                                        * Similar situation to INSERT_IN_PROGRESS case.
+                                        * 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));
                                        xwait = HeapTupleHeaderGetXmax(heapTuple->t_data);
@@ -2086,8 +2087,17 @@ IndexBuildHeapScan(Relation heapRelation,
                                                 * the tuple is dead could lead to missing a
                                                 * uniqueness violation.  In that case we wait for the
                                                 * deleting transaction to finish and check again.
+                                                *
+                                                * Also, if it's a HOT-updated tuple, we should not
+                                                * index it but rather the live tuple at the end of
+                                                * the HOT-chain.  However, the deleting transaction
+                                                * could abort, possibly leaving this tuple as live
+                                                * after all, in which case it has to be indexed. The
+                                                * only way to know what to do is to wait for the
+                                                * deleting transaction to finish and check again.
                                                 */
-                                               if (checking_uniqueness)
+                                               if (checking_uniqueness ||
+                                                       HeapTupleIsHotUpdated(heapTuple))
                                                {
                                                        /*
                                                         * Must drop the lock on the buffer before we wait
@@ -2096,22 +2106,34 @@ 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.
+                                                */
+                                               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;