Don't mark pages all-visible spuriously

Dan Wood diagnosed a long-standing problem that pages containing tuples
that are locked by multixacts containing live lockers may spuriously end
up as candidates for getting their all-visible flag set.  This has the
long-term effect that multixacts remain unfrozen; this may previously
pass undetected, but since commit XYZ it would be reported as
  "ERROR: found multixact 134100944 from before relminmxid 192042633"
because when a later vacuum tries to freeze the page it detects that a
multixact that should have gotten frozen, wasn't.

Dan proposed a (correct) patch that simply sets a variable to its
correct value, after a bogus initialization.  But, per discussion, it
seems better coding to avoid the bogus initializations altogether, since
they could give rise to more bugs later.  Therefore this fix rewrites
the logic a little bit to avoid depending on the bogus initializations.

This bug was part of a family introduced in 9.6 by commit a892234f830e;
later, commit 38e9f90a227d fixed most of them, but this one was
unnoticed.

Authors: Dan Wood, Pavan Deolasee, Álvaro Herrera
Reviewed-by: Masahiko Sawada, Pavan Deolasee, Álvaro Herrera
Discussion: https://postgr.es/m/84EBAC55-F06D-4FBE-A3F3-8BDA093CE3E3@amazon.com

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 1a672150be6e3911483e39239949b57efabe1131..72395a50b8cfea455645c31d525646711461edfd 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6803,9 +6803,10 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
                          xl_heap_freeze_tuple *frz, bool *totally_frozen_p)
 {
    bool        changed = false;
-   bool        freeze_xmax = false;
+   bool        xmax_already_frozen = false;
+   bool        xmin_frozen;
+   bool        freeze_xmax;
    TransactionId xid;
-   bool        totally_frozen = true;
 
    frz->frzflags = 0;
    frz->t_infomask2 = tuple->t_infomask2;
@@ -6814,6 +6815,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
    /* Process xmin */
    xid = HeapTupleHeaderGetXmin(tuple);
+   xmin_frozen = ((xid == FrozenTransactionId) ||
+                  HeapTupleHeaderXminFrozen(tuple));
    if (TransactionIdIsNormal(xid))
    {
        if (TransactionIdPrecedes(xid, relfrozenxid))
@@ -6832,9 +6835,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
            frz->t_infomask |= HEAP_XMIN_FROZEN;
            changed = true;
+           xmin_frozen = true;
        }
-       else
-           totally_frozen = false;
    }
 
    /*
@@ -6857,9 +6859,9 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
                                    relfrozenxid, relminmxid,
                                    cutoff_xid, cutoff_multi, &flags);
 
-       if (flags & FRM_INVALIDATE_XMAX)
-           freeze_xmax = true;
-       else if (flags & FRM_RETURN_IS_XID)
+       freeze_xmax = (flags & FRM_INVALIDATE_XMAX);
+
+       if (flags & FRM_RETURN_IS_XID)
        {
            /*
             * NB -- some of these transformations are only valid because we
@@ -6873,7 +6875,6 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
            if (flags & FRM_MARK_COMMITTED)
                frz->t_infomask |= HEAP_XMAX_COMMITTED;
            changed = true;
-           totally_frozen = false;
        }
        else if (flags & FRM_RETURN_IS_MULTI)
        {
@@ -6895,11 +6896,6 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
            frz->xmax = newxmax;
 
            changed = true;
-           totally_frozen = false;
-       }
-       else
-       {
-           Assert(flags & FRM_NOOP);
        }
    }
    else if (TransactionIdIsNormal(xid))
@@ -6927,11 +6923,24 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
            freeze_xmax = true;
        }
        else
-           totally_frozen = false;
+           freeze_xmax = false;
    }
+   else if ((tuple->t_infomask & HEAP_XMAX_INVALID) ||
+            !TransactionIdIsValid(HeapTupleHeaderGetRawXmax(tuple)))
+   {
+       freeze_xmax = false;
+       xmax_already_frozen = true;
+   }
+   else
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg_internal("found xmax %u (infomask 0x%04x) not frozen, not multi, not normal",
+                                xid, tuple->t_infomask)));
 
    if (freeze_xmax)
    {
+       Assert(!xmax_already_frozen);
+
        frz->xmax = InvalidTransactionId;
 
        /*
@@ -6984,7 +6993,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
        }
    }
 
-   *totally_frozen_p = totally_frozen;
+   *totally_frozen_p = (xmin_frozen &&
+                        (freeze_xmax || xmax_already_frozen));
    return changed;
 }