From ea37a15e424385d76a5976ef14fc24174e738bee Mon Sep 17 00:00:00 2001 From: Matthias van de Meent Date: Mon, 9 Jan 2023 17:34:01 +0100 Subject: [PATCH v1] Prevent underflow of xid8 epoch In some cases where we use xid offsets, it is possible to create a xid4 that is logically "before" the first transaction ID. With xid4, that is not much of an issue because we allow them to over- and underflow as long as the values stay within 2^31 of any active transaction, but this is not the case for xid8: xid8 assumes a strictly increasing counter, which disallows under- and overflows. To still correctly convert xid to xid8, an underflow of the epoch is handled by (depending on context) truncating the value to either FirstNormalFullTransactionId or InvalidFullTransactionId. --- src/backend/replication/walreceiver.c | 21 +++++++++++++++++++-- src/backend/storage/ipc/procarray.c | 11 +++++++++++ src/backend/utils/adt/xid8funcs.c | 12 ++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c index 3876c0188d..5710a42ad6 100644 --- a/src/backend/replication/walreceiver.c +++ b/src/backend/replication/walreceiver.c @@ -1233,10 +1233,27 @@ XLogWalRcvSendHSFeedback(bool immed) nextXid = XidFromFullTransactionId(nextFullXid); xmin_epoch = EpochFromFullTransactionId(nextFullXid); catalog_xmin_epoch = xmin_epoch; + + /* + * If we aren't careful, we can allow the epoch to underflow, resulting + * in all kinds of bad behaviour due to breaking the xid8 sequentiality + * guarantee. So, instead of letting the epoch wrap around, we set the + * xmin to the lowest possible value. + */ if (nextXid < xmin) - xmin_epoch--; + { + if (xmin_epoch == 0) + xmin = InvalidTransactionId; + else + --xmin_epoch; + } if (nextXid < catalog_xmin) - catalog_xmin_epoch--; + { + if (catalog_xmin_epoch == 0) + catalog_xmin = InvalidTransactionId; + else + catalog_xmin_epoch--; + } elog(DEBUG2, "sending hot standby feedback xmin %u epoch %u catalog_xmin %u catalog_xmin_epoch %u", xmin, xmin_epoch, catalog_xmin, catalog_xmin_epoch); diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 4340bf9641..f83ccacf03 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -4305,6 +4305,9 @@ GlobalVisCheckRemovableXid(Relation rel, TransactionId xid) * held by the backend and xid is from a table (where vacuum/freezing ensures * the xid has to be within that range), or if xid is from the procarray and * prevents xid wraparound that way. + * + * The function returns FirstNormalTransactionId if provided with a xid + * that would make the epoch underflow and wrap around. */ static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel, TransactionId xid) @@ -4317,6 +4320,14 @@ FullXidRelativeTo(FullTransactionId rel, TransactionId xid) /* not guaranteed to find issues, but likely to catch mistakes */ AssertTransactionIdInAllowableRange(xid); + /* + * An epoch of 0 would underflow on this condition and break the + * 'xid8 does not wrap around' guarantee. + */ + if (unlikely(EpochFromFullTransactionId(rel) == 0 && + ((int32) (xid - rel_xid)) < 0)) + return FirstNormalFullTransactionId; + return FullTransactionIdFromU64(U64FromFullTransactionId(rel) + (int32) (xid - rel_xid)); } diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c index e616303a29..55fb838330 100644 --- a/src/backend/utils/adt/xid8funcs.c +++ b/src/backend/utils/adt/xid8funcs.c @@ -153,6 +153,9 @@ TransactionIdInRecentPast(FullTransactionId fxid, TransactionId *extracted_xid) * FullTransactionId. Use next_fxid as a reference FullTransactionId, so that * we can compute the high order bits. It must have been obtained by the * caller with ReadNextFullTransactionId() after the snapshot was created. + * + * The function returns FirstNormalTransactionId if provided with a xid + * that would make the epoch underflow and wrap around. */ static FullTransactionId widen_snapshot_xid(TransactionId xid, FullTransactionId next_fxid) @@ -172,7 +175,16 @@ widen_snapshot_xid(TransactionId xid, FullTransactionId next_fxid) * more than one epoch ahead of the TransactionIds in any snapshot. */ if (xid > next_xid) + { + /* + * We may not allow the epoch to underflow, so instead we'll output + * FirstNormalFullTransactionId as a substitute. + */ + if (epoch == 0) + return FirstNormalFullTransactionId; + epoch--; + } return FullTransactionIdFromEpochAndXid(epoch, xid); } -- 2.30.2