]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
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:05 +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 a7dffd151ab9e5f5e621d0f5d5534e103bc120ab..f5db3c775c3417e3f7923d92ee5756adb5f68942 100644 (file)
@@ -1718,7 +1718,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;
 
                /*
@@ -1887,7 +1888,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;
@@ -1919,39 +1920,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;
-
-       /*
-        * 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.
-        */
-       if (TransactionIdEquals(xmin, FrozenTransactionId) &&
-               TransactionIdIsValid(xmax))
-               return true;
-
-       return false;
-}
 
 /*
  * UpdateXmaxHintBits - update tuple hint bits after xmax transaction ends
@@ -5077,7 +5045,8 @@ l4:
                 * the end of the chain, we're done, so return success.
                 */
                if (TransactionIdIsValid(priorXmax) &&
-                       !HeapTupleUpdateXmaxMatchesXmin(priorXmax, mytup.t_data))
+                       !TransactionIdEquals(HeapTupleHeaderGetXmin(mytup.t_data),
+                                                                priorXmax))
                {
                        UnlockReleaseBuffer(buf);
                        return HeapTupleMayBeUpdated;
@@ -5520,26 +5489,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))
-                               {
-                                       xid = FrozenTransactionId;
-                                       *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
                        {
@@ -5610,17 +5567,18 @@ 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.
+                        * aborted then it's okay to ignore it, otherwise not.  However,
+                        * if the Xid is older than the cutoff_xid, we must remove it.
+                        * Note that such an old updater cannot possibly be committed,
+                        * because HeapTupleSatisfiesVacuum would have returned
+                        * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
+                        *
+                        * Note the TransactionIdDidAbort() test is just an optimization
+                        * and not strictly necessary for correctness.
                         *
                         * As with all tuple visibility routines, it's critical to test
-                        * TransactionIdIsInProgress before TransactionIdDidCommit,
+                        * TransactionIdIsInProgress before the transam.c routines,
                         * 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))
@@ -5628,33 +5586,46 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
                                Assert(!TransactionIdIsValid(update_xid));
                                update_xid = xid;
                        }
-                       else if (TransactionIdDidCommit(xid))
+                       else if (!TransactionIdDidAbort(xid))
                        {
                                /*
-                                * The transaction committed, so we can tell caller to set
-                                * HEAP_XMAX_COMMITTED.  (We can only do this because we know
-                                * the transaction is not running.)
+                                * Test whether to tell caller to set HEAP_XMAX_COMMITTED
+                                * while we have the Xid still in cache.  Note this can only
+                                * be done if the transaction is known not running.
                                 */
+                               if (TransactionIdDidCommit(xid))
+                                       update_committed = true;
                                Assert(!TransactionIdIsValid(update_xid));
-                               update_committed = true;
                                update_xid = xid;
                        }
 
-                       /*
-                        * Not in progress, not committed -- must be aborted or crashed;
-                        * we can ignore it.
-                        */
-
                        /*
                         * If we determined that it's an Xid corresponding to an update
                         * that must be retained, additionally add it to the list of
-                        * members of the new Multi, in case we end up using that.  (We
+                        * members of the new Multis, in case we end up using that.  (We
                         * might still decide to use only an update Xid and not a multi,
                         * but it's easier to maintain the list as we walk the old members
                         * list.)
+                        *
+                        * It is possible to end up with a very old updater Xid that
+                        * crashed and thus did not mark itself as aborted in pg_clog.
+                        * That would manifest as a pre-cutoff Xid.  Make sure to ignore
+                        * it.
                         */
                        if (TransactionIdIsValid(update_xid))
-                               newmembers[nnewmembers++] = members[i];
+                       {
+                               if (!TransactionIdPrecedes(update_xid, cutoff_xid))
+                               {
+                                       newmembers[nnewmembers++] = members[i];
+                               }
+                               else
+                               {
+                                       /* cannot have committed: would be HEAPTUPLE_DEAD */
+                                       Assert(!TransactionIdDidCommit(update_xid));
+                                       update_xid = InvalidTransactionId;
+                                       update_committed = false;
+                               }
+                       }
                }
                else
                {
@@ -5721,10 +5692,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
@@ -5835,18 +5803,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
        else if (TransactionIdIsNormal(xid) &&
                         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;
-                       changed = true;
-                       /* must not freeze */
-               }
-               else
-                       freeze_xmax = true;
+               freeze_xmax = true;
        }
 
        if (freeze_xmax)
index a342f57f42ff4b3275216842190083c876d4c3a1..9a8db74cb95ea1356df8dff8984d4a4459100ce9 100644 (file)
@@ -435,7 +435,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;
 
                /*
@@ -774,7 +774,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 abef418106840d2dd53f04dfe79160dc00ca04a6..6ece3591e4f189b7019b193a7f64b3060bcb959f 100644 (file)
@@ -1678,15 +1678,15 @@ 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++;
+       if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
+       {
+               vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;
+               vacrelstats->num_dead_tuples++;
+       }
 }
 
 /*
index 79e4701bb8ae7d0693860ccee2c1acaa065995b3..315a555dcff2b21f4680433c1ffd7c363d887d9c 100644 (file)
@@ -2019,7 +2019,8 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode,
                         * buffer's content lock, since xmin never changes in an existing
                         * tuple.)
                         */
-                       if (!HeapTupleUpdateXmaxMatchesXmin(priorXmax, tuple.t_data))
+                       if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple.t_data),
+                                                                        priorXmax))
                        {
                                ReleaseBuffer(buffer);
                                return NULL;
@@ -2137,7 +2138,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 93a481f483189f5af4993e1554b65ed921f89443..4a187f899cf8dabaf0bdc6521ee0e0e47c8535ac 100644 (file)
@@ -127,9 +127,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);
 
index 07cff08b337805891996f5dbc690f935ceb01b60..46134c4a77eb7f629a6f922650fa2f0d935da49f 100644 (file)
@@ -30,6 +30,5 @@ test: lock-committed-keyupdate
 test: update-locked-tuple
 test: propagate-lock-delete
 test: tuplelock-conflict
-test: freeze-the-dead
 test: drop-index-concurrently-1
 test: timeouts