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 ad4cdedcffa7013091dd6ab02bde85a8dbd2a577..5016181fd70d6c0da7916cd8613f3a97aaf380bc 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6676,9 +6676,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;
@@ -6687,6 +6688,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))
@@ -6705,9 +6708,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
 
                        frz->t_infomask |= HEAP_XMIN_FROZEN;
                        changed = true;
+                       xmin_frozen = true;
                }
-               else
-                       totally_frozen = false;
        }
 
        /*
@@ -6730,9 +6732,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
@@ -6746,7 +6748,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)
                {
@@ -6768,11 +6769,6 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
                        frz->xmax = newxmax;
 
                        changed = true;
-                       totally_frozen = false;
-               }
-               else
-               {
-                       Assert(flags & FRM_NOOP);
                }
        }
        else if (TransactionIdIsNormal(xid))
@@ -6800,11 +6796,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;
 
                /*
@@ -6857,7 +6866,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
                }
        }
 
-       *totally_frozen_p = totally_frozen;
+       *totally_frozen_p = (xmin_frozen &&
+                                                (freeze_xmax || xmax_already_frozen));
        return changed;
 }