From 811416670a11e8cfa7809fa5dc1bc522e3dc7b88 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Thu, 29 Dec 2022 11:54:30 -0800 Subject: [PATCH v2] Check xmin/xmax commit status when freezing executes. --- src/include/access/heapam.h | 10 ++++ src/backend/access/heap/heapam.c | 83 +++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 09a1993f4..1644985e1 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -100,6 +100,14 @@ typedef enum HEAPTUPLE_DELETE_IN_PROGRESS /* deleting xact is still in progress */ } HTSV_Result; +/* + * heap_prepare_freeze_tuple requests that heap_freeze_execute_prepared check + * xmin committed and/or xmax aborted. These are delayed until freeze plan + * execution to avoid unnecessary clog lookups. + */ +#define HEAP_FREEZE_XMIN_CHECK 0x01 +#define HEAP_FREEZE_XMAX_CHECK 0x02 + /* heap_prepare_freeze_tuple state describing how to freeze a tuple */ typedef struct HeapTupleFreeze { @@ -109,6 +117,8 @@ typedef struct HeapTupleFreeze uint16 t_infomask; uint8 frzflags; + /* xmin/xmax check flags */ + uint8 checkflags; /* Page offset number for tuple */ OffsetNumber offset; } HeapTupleFreeze; diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 34d83dc70..353733aa3 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -6502,10 +6502,11 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, freeze_xmax = false; TransactionId xid; - frz->frzflags = 0; + frz->xmax = HeapTupleHeaderGetRawXmax(tuple); frz->t_infomask2 = tuple->t_infomask2; frz->t_infomask = tuple->t_infomask; - frz->xmax = HeapTupleHeaderGetRawXmax(tuple); + frz->frzflags = 0; + frz->checkflags = 0; /* * Process xmin, while keeping track of whether it's already frozen, or @@ -6523,14 +6524,12 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, errmsg_internal("found xmin %u from before relfrozenxid %u", xid, cutoffs->relfrozenxid))); - freeze_xmin = TransactionIdPrecedes(xid, cutoffs->OldestXmin); - if (freeze_xmin && !TransactionIdDidCommit(xid)) - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg_internal("uncommitted xmin %u from before xid cutoff %u needs to be frozen", - xid, cutoffs->OldestXmin))); - /* Will set freeze_xmin flags in freeze plan below */ + freeze_xmin = TransactionIdPrecedes(xid, cutoffs->OldestXmin); + + /* Verify that xmin committed if and when freeze plan is executed */ + if (freeze_xmin) + frz->checkflags |= HEAP_FREEZE_XMIN_CHECK; } /* @@ -6553,7 +6552,7 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, } /* Now process xmax */ - xid = HeapTupleHeaderGetRawXmax(tuple); + xid = frz->xmax; if (tuple->t_infomask & HEAP_XMAX_IS_MULTI) { /* Raw xmax is a MultiXactId */ @@ -6664,21 +6663,16 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, errmsg_internal("found xmax %u from before relfrozenxid %u", xid, cutoffs->relfrozenxid))); - if (TransactionIdPrecedes(xid, cutoffs->OldestXmin)) - freeze_xmax = true; + /* Will set freeze_xmax flags in freeze plan below */ + freeze_xmax = TransactionIdPrecedes(xid, cutoffs->OldestXmin); /* - * 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). + * Verify that xmax aborted if and when freeze plan is executed, + * provided it's from an update. (A lock-only xmax can be removed + * independent of this, since the lock is released at xact end.) */ - if (freeze_xmax && !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask) && - TransactionIdDidCommit(xid)) - ereport(ERROR, - (errcode(ERRCODE_DATA_CORRUPTED), - errmsg_internal("cannot freeze committed xmax %u", - xid))); + if (freeze_xmax && !HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)) + frz->checkflags |= HEAP_FREEZE_XMAX_CHECK; } else if (!TransactionIdIsValid(xid)) { @@ -6806,17 +6800,58 @@ heap_freeze_execute_prepared(Relation rel, Buffer buffer, Assert(ntuples > 0); + /* + * Perform xmin/xmax commit status sanity checks before critical section. + * + * These checks are delayed until execution time to avoid needlessly + * checking commit status before it's really necessary. + */ + for (int i = 0; i < ntuples; i++) + { + HeapTupleFreeze *frz = tuples + i; + ItemId itemid = PageGetItemId(page, frz->offset); + HeapTupleHeader htup; + + htup = (HeapTupleHeader) PageGetItem(page, itemid); + + /* Verify that xmin committed when that's expected */ + if (frz->checkflags & HEAP_FREEZE_XMIN_CHECK) + { + TransactionId xmin = HeapTupleHeaderGetRawXmin(htup); + + Assert(!HeapTupleHeaderXminFrozen(htup)); + if (unlikely(!TransactionIdDidCommit(xmin))) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("uncommitted xmin %u needs to be frozen", + xmin))); + } + /* Verify that xmax aborted when that's expected */ + if (frz->checkflags & HEAP_FREEZE_XMAX_CHECK) + { + TransactionId xmax = HeapTupleHeaderGetRawXmax(htup); + + Assert(TransactionIdIsNormal(xmax)); + if (unlikely(!TransactionIdDidAbort(xmax))) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("cannot freeze nonaborted xmax %u", + xmax))); + } + } + START_CRIT_SECTION(); MarkBufferDirty(buffer); for (int i = 0; i < ntuples; i++) { + HeapTupleFreeze *frz = tuples + i; + ItemId itemid = PageGetItemId(page, frz->offset); HeapTupleHeader htup; - ItemId itemid = PageGetItemId(page, tuples[i].offset); htup = (HeapTupleHeader) PageGetItem(page, itemid); - heap_execute_freeze_tuple(htup, &tuples[i]); + heap_execute_freeze_tuple(htup, frz); } /* Now WAL-log freezing if necessary */ -- 2.38.1