From d2599ecfcc74fea9fad1720a70210a740c716730 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 4 May 2018 15:24:44 -0300 Subject: [PATCH] Don't mark pages all-visible spuriously MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 --- src/backend/access/heap/heapam.c | 40 ++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 1a672150be..72395a50b8 100644 --- 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; } -- 2.40.0