Revert bogus fixes of HOT-freezing bug
authorAlvaro Herrera <alvherre@alvh.no-ip.org>
Thu, 2 Nov 2017 14:51:05 +0000 (15:51 +0100)
committerAlvaro Herrera <alvherre@alvh.no-ip.org>
Thu, 2 Nov 2017 14:51:41 +0000 (15:51 +0100)
It turns out we misdiagnosed what the real problem was.  Revert the
previous changes, because they may have worse consequences going
forward.  A better fix is forthcoming.

The simplistic test case is kept, though disabled.

Discussion: https://postgr.es/m/20171102112019.33wb7g5wp4zpjelu@alap3.anarazel.de

src/backend/access/heap/heapam.c
src/backend/access/heap/pruneheap.c
src/backend/commands/vacuumlazy.c
src/backend/executor/execMain.c
src/include/access/heapam.h
src/test/isolation/isolation_schedule

index 52dda41cc439223e86b130c4a1715849220d9c12..765750b874372cef25a6d0a2b0d37168761a64f3 100644 (file)
@@ -2074,7 +2074,8 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
         * broken.
         */
        if (TransactionIdIsValid(prev_xmax) &&
-           !HeapTupleUpdateXmaxMatchesXmin(prev_xmax, heapTuple->t_data))
+           !TransactionIdEquals(prev_xmax,
+                                HeapTupleHeaderGetXmin(heapTuple->t_data)))
            break;
 
        /*
@@ -2260,7 +2261,7 @@ heap_get_latest_tid(Relation relation,
         * tuple.  Check for XMIN match.
         */
        if (TransactionIdIsValid(priorXmax) &&
-           !HeapTupleUpdateXmaxMatchesXmin(priorXmax, tp.t_data))
+           !TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(tp.t_data)))
        {
            UnlockReleaseBuffer(buffer);
            break;
@@ -2292,50 +2293,6 @@ heap_get_latest_tid(Relation relation,
    }                           /* end of loop */
 }
 
-/*
- * HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage
- *
- * Given the new version of a tuple after some update, verify whether the
- * given Xmax (corresponding to the previous version) matches the tuple's
- * Xmin, taking into account that the Xmin might have been frozen after the
- * update.
- */
-bool
-HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
-{
-   TransactionId   xmin = HeapTupleHeaderGetXmin(htup);
-
-   /*
-    * If the xmax of the old tuple is identical to the xmin of the new one,
-    * it's a match.
-    */
-   if (TransactionIdEquals(xmax, xmin))
-       return true;
-
-   /*
-    * If the Xmin that was in effect prior to a freeze matches the Xmax,
-    * it's good too.
-    */
-   if (HeapTupleHeaderXminFrozen(htup) &&
-       TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax))
-       return true;
-
-   /*
-    * When a tuple is frozen, the original Xmin is lost, but we know it's a
-    * committed transaction.  So unless the Xmax is InvalidXid, we don't know
-    * for certain that there is a match, but there may be one; and we must
-    * return true so that a HOT chain that is half-frozen can be walked
-    * correctly.
-    *
-    * We no longer freeze tuples this way, but we must keep this in order to
-    * interpret pre-pg_upgrade pages correctly.
-    */
-   if (TransactionIdEquals(xmin, FrozenTransactionId) &&
-       TransactionIdIsValid(xmax))
-       return true;
-
-   return false;
-}
 
 /*
  * UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends
@@ -5755,7 +5712,8 @@ l4:
         * end of the chain, we're done, so return success.
         */
        if (TransactionIdIsValid(priorXmax) &&
-           !HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data))
+           !TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data),
+                                priorXmax))
        {
            result = HeapTupleMayBeUpdated;
            goto out_locked;
@@ -6449,23 +6407,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
            Assert(TransactionIdIsValid(xid));
 
            /*
-            * The updating transaction cannot possibly be still running, but
-            * verify whether it has committed, and request to set the
-            * COMMITTED flag if so.  (We normally don't see any tuples in
-            * this state, because they are removed by page pruning before we
-            * try to freeze the page; but this can happen if the updating
-            * transaction commits after the page is pruned but before
-            * HeapTupleSatisfiesVacuum).
+            * If the xid is older than the cutoff, it has to have aborted,
+            * otherwise the tuple would have gotten pruned away.
             */
            if (TransactionIdPrecedes(xid, cutoff_xid))
            {
-               if (TransactionIdDidCommit(xid))
-                   *flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;
-               else
-               {
-                   *flags |= FRM_INVALIDATE_XMAX;
-                   xid = InvalidTransactionId; /* not strictly necessary */
-               }
+               Assert(!TransactionIdDidCommit(xid));
+               *flags |= FRM_INVALIDATE_XMAX;
+               xid = InvalidTransactionId; /* not strictly necessary */
            }
            else
            {
@@ -6538,16 +6487,13 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
            /*
             * It's an update; should we keep it?  If the transaction is known
             * aborted or crashed then it's okay to ignore it, otherwise not.
+            * Note that an updater older than cutoff_xid cannot possibly be
+            * committed, because HeapTupleSatisfiesVacuum would have returned
+            * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
             *
             * As with all tuple visibility routines, it's critical to test
             * TransactionIdIsInProgress before TransactionIdDidCommit,
             * because of race conditions explained in detail in tqual.c.
-            *
-            * We normally don't see committed updating transactions earlier
-            * than the cutoff xid, because they are removed by page pruning
-            * before we try to freeze the page; but it can happen if the
-            * updating transaction commits after the page is pruned but
-            * before HeapTupleSatisfiesVacuum.
             */
            if (TransactionIdIsCurrentTransactionId(xid) ||
                TransactionIdIsInProgress(xid))
@@ -6572,6 +6518,13 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
             * we can ignore it.
             */
 
+           /*
+            * Since the tuple wasn't marked HEAPTUPLE_DEAD by vacuum, the
+            * update Xid cannot possibly be older than the xid cutoff.
+            */
+           Assert(!TransactionIdIsValid(update_xid) ||
+                  !TransactionIdPrecedes(update_xid, cutoff_xid));
+
            /*
             * If we determined that it's an Xid corresponding to an update
             * that must be retained, additionally add it to the list of
@@ -6650,10 +6603,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
  *
  * It is assumed that the caller has checked the tuple with
  * HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
- * (else we should be removing the tuple, not freezing it).  However, note
- * that we don't remove HOT tuples even if they are dead, and it'd be incorrect
- * to freeze them (because that would make them visible), so we mark them as
- * update-committed, and needing further freezing later on.
+ * (else we should be removing the tuple, not freezing it).
  *
  * NB: cutoff_xid *must* be <= the current global xmin, to ensure that any
  * XID older than it could neither be running nor seen as running by any
@@ -6764,22 +6714,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
    else if (TransactionIdIsNormal(xid))
    {
        if (TransactionIdPrecedes(xid, cutoff_xid))
-       {
-           /*
-            * Must freeze regular XIDs older than the cutoff.  We must not
-            * freeze a HOT-updated tuple, though; doing so would bring it
-            * back to life.
-            */
-           if (HeapTupleHeaderIsHotUpdated(tuple))
-           {
-               frz->t_infomask |= HEAP_XMAX_COMMITTED;
-               totally_frozen = false;
-               changed = true;
-               /* must not freeze */
-           }
-           else
-               freeze_xmax = true;
-       }
+           freeze_xmax = true;
        else
            totally_frozen = false;
    }
index 7753ee7b120924f10c5e5b1c382d997a0363948c..52231ac41784d8713b705361f165e695faa16bba 100644 (file)
@@ -473,7 +473,7 @@ heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum,
         * Check the tuple XMIN against prior XMAX, if any
         */
        if (TransactionIdIsValid(priorXmax) &&
-           !HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
+           !TransactionIdEquals(HeapTupleHeaderGetXmin(htup), priorXmax))
            break;
 
        /*
@@ -813,7 +813,7 @@ heap_get_root_tuples(Page page, OffsetNumber *root_offsets)
            htup = (HeapTupleHeader) PageGetItem(page, lp);
 
            if (TransactionIdIsValid(priorXmax) &&
-               !HeapTupleUpdateXmaxMatchesXmin(priorXmax, htup))
+               !TransactionIdEquals(priorXmax, HeapTupleHeaderGetXmin(htup)))
                break;
 
            /* Remember the root line pointer for this item */
index 172d213fdb85ad0ee0fc44fb72b1999870423fe9..6587db77acff1452772b16c30ee634689acdb7ae 100644 (file)
@@ -2029,17 +2029,17 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats,
                       ItemPointer itemptr)
 {
    /*
-    * The array must never overflow, since we rely on all deletable tuples
-    * being removed; inability to remove a tuple might cause an old XID to
-    * persist beyond the freeze limit, which could be disastrous later on.
+    * The array shouldn't overflow under normal behavior, but perhaps it
+    * could if we are given a really small maintenance_work_mem. In that
+    * case, just forget the last few tuples (we'll get 'em next time).
     */
-   if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
-       elog(ERROR, "dead tuple array overflow");
-
-   vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
-   vacrelstats->num_dead_tuples++;
-   pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
-                                vacrelstats->num_dead_tuples);
+   if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
+   {
+       vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
+       vacrelstats->num_dead_tuples++;
+       pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
+                                    vacrelstats->num_dead_tuples);
+   }
 }
 
 /*
index 638a856dc392dbeba532a0a3da0e1c8288861eae..2e8aca59a7fde9fca324f028d54ab8b22325aa04 100644 (file)
@@ -2595,7 +2595,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
             * atomic, and Xmin never changes in an existing tuple, except to
             * invalid or frozen, and neither of those can match priorXmax.)
             */
-           if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
+           if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
+                                    priorXmax))
            {
                ReleaseBuffer(buffer);
                return NULL;
@@ -2742,7 +2743,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
        /*
         * As above, if xmin isn't what we're expecting, do nothing.
         */
-       if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
+       if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
+                                priorXmax))
        {
            ReleaseBuffer(buffer);
            return NULL;
index 9f4367d704e62fb9023408322dacecda0365568b..4e41024e9260701445cdd1daa0608ccb2e4962a2 100644 (file)
@@ -146,9 +146,6 @@ extern void heap_get_latest_tid(Relation relation, Snapshot snapshot,
                    ItemPointer tid);
 extern void setLastTid(const ItemPointer tid);
 
-extern bool HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax,
-                              HeapTupleHeader htup);
-
 extern BulkInsertState GetBulkInsertState(void);
 extern void FreeBulkInsertState(BulkInsertState);
 extern void ReleaseBulkInsertStatePin(BulkInsertState bistate);
index 7dad3c23163d0f22cdb12a0254a2b28225ed1112..32c965b2a02a44d6a9daef2e3e1bb8123953ca8f 100644 (file)
@@ -44,7 +44,6 @@ test: update-locked-tuple
 test: propagate-lock-delete
 test: tuplelock-conflict
 test: tuplelock-update
-test: freeze-the-dead
 test: nowait
 test: nowait-2
 test: nowait-3