From 3f432d15aa9550112fff18a6670e4b75119ddbb6 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 13 Nov 2017 18:45:47 -0800 Subject: [PATCH 2/2] Perform a lot more sanity checks when freezing tuples. The previous commit has shown that the sanity checks around freezing aren't strong enough. Strengthening them seems especially important because the existance of the bug has caused corruption that we don't want to make even worse during future vacuum cycles. Author: Andres Freund Discussion: https://postgr.es/m/20171102112019.33wb7g5wp4zpjelu@alap3.anarazel.de Backpatch: 9.3- --- src/backend/access/heap/heapam.c | 74 ++++++++++++++++++++++++++++------- src/backend/access/heap/rewriteheap.c | 5 ++- src/backend/commands/vacuumlazy.c | 15 ++++++- src/include/access/heapam.h | 5 ++- src/include/access/heapam_xlog.h | 2 + 5 files changed, 81 insertions(+), 20 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 3acef279f47..eb93718baa7 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -6357,6 +6357,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple) */ static TransactionId FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, + TransactionId relfrozenxid, TransactionId relminmxid, TransactionId cutoff_xid, MultiXactId cutoff_multi, uint16 *flags) { @@ -6385,14 +6386,19 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, } else if (MultiXactIdPrecedes(multi, cutoff_multi)) { + if (MultiXactIdPrecedes(multi, relminmxid)) + elog(ERROR, "encountered multixact from before horizon"); + /* * This old multi cannot possibly have members still running. If it * was a locker only, it can be removed without any further * consideration; but if it contained an update, we might need to * preserve it. */ - Assert(!MultiXactIdIsRunning(multi, - HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))); + if (MultiXactIdIsRunning(multi, + HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))) + elog(ERROR, "multixact from before cutoff found to be still running"); + if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)) { *flags |= FRM_INVALIDATE_XMAX; @@ -6412,7 +6418,10 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, */ if (TransactionIdPrecedes(xid, cutoff_xid)) { - Assert(!TransactionIdDidCommit(xid)); + if (TransactionIdPrecedes(xid, relfrozenxid)) + elog(ERROR, "encountered xid from before horizon"); + if (TransactionIdDidCommit(xid)) + elog(ERROR, "can't freeze committed xmax"); *flags |= FRM_INVALIDATE_XMAX; xid = InvalidTransactionId; /* not strictly necessary */ } @@ -6484,6 +6493,10 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, { TransactionId xid = members[i].xid; + Assert(TransactionIdIsValid(xid)); + if (TransactionIdPrecedes(xid, relfrozenxid)) + elog(ERROR, "encountered xid from before horizon"); + /* * 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. @@ -6512,18 +6525,24 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, update_committed = true; update_xid = xid; } + else + { + /* + * Not in progress, not committed -- must be aborted or crashed; + * we can ignore it. + */ + } - /* - * Not in progress, not committed -- must be aborted or crashed; - * 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. + * update Xid cannot possibly be older than the xid cutoff. The + * presence of such a tuple would cause corruption, so be paranoid + * and check. */ - Assert(!TransactionIdIsValid(update_xid) || - !TransactionIdPrecedes(update_xid, cutoff_xid)); + if (TransactionIdIsValid(update_xid) && + TransactionIdPrecedes(update_xid, cutoff_xid)) + elog(ERROR, "encountered xid from before xid cutoff"); /* * If we determined that it's an Xid corresponding to an update @@ -6620,8 +6639,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, * recovery. We really need to remove old xids. */ bool -heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, - TransactionId cutoff_multi, +heap_prepare_freeze_tuple(HeapTupleHeader tuple, + TransactionId relfrozenxid, TransactionId relminmxid, + TransactionId cutoff_xid, TransactionId cutoff_multi, xl_heap_freeze_tuple *frz, bool *totally_frozen_p) { bool changed = false; @@ -6640,6 +6660,12 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, { if (TransactionIdPrecedes(xid, cutoff_xid)) { + if (TransactionIdPrecedes(xid, relfrozenxid)) + elog(ERROR, "encountered xid from before xid cutoff"); + + if (!TransactionIdDidCommit(xid)) + elog(ERROR, "uncommitted xmin needs to be frozen"); + frz->t_infomask |= HEAP_XMIN_FROZEN; changed = true; } @@ -6664,6 +6690,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, uint16 flags; newxmax = FreezeMultiXactId(xid, tuple->t_infomask, + relfrozenxid, relminmxid, cutoff_xid, cutoff_multi, &flags); if (flags & FRM_INVALIDATE_XMAX) @@ -6714,7 +6741,21 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, else if (TransactionIdIsNormal(xid)) { if (TransactionIdPrecedes(xid, cutoff_xid)) + { + if (TransactionIdPrecedes(xid, relfrozenxid)) + elog(ERROR, "encountered xid from before xid cutoff"); + + /* + * If we freeze xmax, make absolutely sure that it's not an XID + * that is important (Note, a lock-only xmax can be removed + * independent of committedness, since a committed lock holder has + * released the lock). + */ + if (!(tuple->t_infomask & HEAP_XMAX_LOCK_ONLY) && + TransactionIdDidCommit(xid)) + elog(ERROR, "can't freeze committed xmax"); freeze_xmax = true; + } else totally_frozen = false; } @@ -6819,14 +6860,17 @@ heap_execute_freeze_tuple(HeapTupleHeader tuple, xl_heap_freeze_tuple *frz) * Useful for callers like CLUSTER that perform their own WAL logging. */ bool -heap_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, - TransactionId cutoff_multi) +heap_freeze_tuple(HeapTupleHeader tuple, + TransactionId relfrozenxid, TransactionId relminmxid, + TransactionId cutoff_xid, TransactionId cutoff_multi) { xl_heap_freeze_tuple frz; bool do_freeze; bool tuple_totally_frozen; - do_freeze = heap_prepare_freeze_tuple(tuple, cutoff_xid, cutoff_multi, + do_freeze = heap_prepare_freeze_tuple(tuple, + relfrozenxid, relminmxid, + cutoff_xid, cutoff_multi, &frz, &tuple_totally_frozen); /* diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c index f93c194e182..7d163c91379 100644 --- a/src/backend/access/heap/rewriteheap.c +++ b/src/backend/access/heap/rewriteheap.c @@ -407,7 +407,10 @@ rewrite_heap_tuple(RewriteState state, * While we have our hands on the tuple, we may as well freeze any * eligible xmin or xmax, so that future VACUUM effort can be saved. */ - heap_freeze_tuple(new_tuple->t_data, state->rs_freeze_xid, + heap_freeze_tuple(new_tuple->t_data, + state->rs_old_rel->rd_rel->relfrozenxid, + state->rs_old_rel->rd_rel->relminmxid, + state->rs_freeze_xid, state->rs_cutoff_multi); /* diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 6587db77acf..4a01137527c 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -467,6 +467,8 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, blkno; HeapTupleData tuple; char *relname; + TransactionId relfrozenxid = onerel->rd_rel->relfrozenxid; + TransactionId relminmxid = onerel->rd_rel->relminmxid; BlockNumber empty_pages, vacuumed_pages; double num_tuples, @@ -1004,6 +1006,13 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, * tuple, we choose to keep it, because it'll be a lot * cheaper to get rid of it in the next pruning pass than * to treat it like an indexed tuple. + * + * If this were to happen for a tuple that actually needed + * to be deleted , we'd be in trouble, because it'd + * possibly leave a tuple below the relation's xmin + * horizon alive. But checks in + * heap_prepare_freeze_tuple() would trigger in that case, + * preventing corruption. */ if (HeapTupleIsHotUpdated(&tuple) || HeapTupleIsHeapOnly(&tuple)) @@ -1095,8 +1104,10 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats, * Each non-removable tuple must be checked to see if it needs * freezing. Note we already have exclusive buffer lock. */ - if (heap_prepare_freeze_tuple(tuple.t_data, FreezeLimit, - MultiXactCutoff, &frozen[nfrozen], + if (heap_prepare_freeze_tuple(tuple.t_data, + relfrozenxid, relminmxid, + FreezeLimit, MultiXactCutoff, + &frozen[nfrozen], &tuple_totally_frozen)) frozen[nfrozen++].offset = offnum; diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 4e41024e926..f1366ed9581 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -168,8 +168,9 @@ extern HTSU_Result heap_lock_tuple(Relation relation, HeapTuple tuple, bool follow_update, Buffer *buffer, HeapUpdateFailureData *hufd); extern void heap_inplace_update(Relation relation, HeapTuple tuple); -extern bool heap_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, - TransactionId cutoff_multi); +extern bool heap_freeze_tuple(HeapTupleHeader tuple, + TransactionId relfrozenxid, TransactionId relminmxid, + TransactionId cutoff_xid, TransactionId cutoff_multi); extern bool heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid, MultiXactId cutoff_multi, Buffer buf); extern bool heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple); diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index 81a6a395c4f..ad5f982978c 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -390,6 +390,8 @@ extern XLogRecPtr log_heap_freeze(Relation reln, Buffer buffer, TransactionId cutoff_xid, xl_heap_freeze_tuple *tuples, int ntuples); extern bool heap_prepare_freeze_tuple(HeapTupleHeader tuple, + TransactionId relfrozenxid, + TransactionId relminmxid, TransactionId cutoff_xid, TransactionId cutoff_multi, xl_heap_freeze_tuple *frz, -- 2.14.1.536.g6867272d5b.dirty