Hello, Andres.
> Not sure I like TransactionIdRetreatSafely() as a name. Maybe
> TransactionIdRetreatClamped() is better?
I think it is better to just replace TransactionIdRetreatedBy.
It is not used anymore after
`v3-0001-WIP-Fix-corruption-due-to-vacuum_defer_cleanup_ag.patch` -
so, it is better to replace the dangerous version in order to avoid
similar issues in the future.
But we need also to move FullXidRelativeTo in that case (not sure it is safe).
> I think it'll make the code easier to read in the other places too, the
> variable names / function names in this space are uncomfortably long to
> fit into 78chars..., particularly when there's two references to the
> same variable in the same line.
Looks fine for my taste, but it is pretty far from perfect :)
Best regards,
Michail.
From ffa80fece3384e3a12a52de18102a0d169f52841 Mon Sep 17 00:00:00 2001
From: Nkey <michail.nikol...@gmail.com>
Date: Sat, 4 Feb 2023 15:16:52 +0300
Subject: [PATCH] WIP: Fix corruption due to vacuum_defer_cleanup_age
underflowing 64bit xids
---
src/backend/storage/ipc/procarray.c | 56 ++++++++++++-----------------
src/include/access/transam.h | 71 +++++++++++++++++++++++++++++++++----
2 files changed, 87 insertions(+), 40 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index 4340bf9641..5dd662cd75 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -368,8 +368,6 @@ static void ProcArrayGroupClearXid(PGPROC *proc,
TransactionId latestXid);
static void MaintainLatestCompletedXid(TransactionId latestXid);
static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
-static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
-
TransactionId xid);
static void GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons);
/*
@@ -1889,16 +1887,32 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
* in varsup.c. Also note that we intentionally don't apply
* vacuum_defer_cleanup_age on standby servers.
*/
+
Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
+
h->shared_oldest_nonremovable));
+
Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
+
h->data_oldest_nonremovable));
+
h->oldest_considered_running =
TransactionIdRetreatedBy(h->oldest_considered_running,
-
vacuum_defer_cleanup_age);
- h->shared_oldest_nonremovable =
+
vacuum_defer_cleanup_age,
+
h->latest_completed);
+
+ h->shared_oldest_nonremovable =
TransactionIdRetreatedBy(h->shared_oldest_nonremovable,
-
vacuum_defer_cleanup_age);
- h->data_oldest_nonremovable =
+
vacuum_defer_cleanup_age,
+
h->latest_completed);
+
+ h->data_oldest_nonremovable =
TransactionIdRetreatedBy(h->data_oldest_nonremovable,
-
vacuum_defer_cleanup_age);
+
vacuum_defer_cleanup_age,
+
h->latest_completed);
+
/* defer doesn't apply to temp relations */
+
+
Assert(TransactionIdPrecedesOrEquals(h->oldest_considered_running,
+
h->shared_oldest_nonremovable));
+
Assert(TransactionIdPrecedesOrEquals(h->shared_oldest_nonremovable,
+
h->data_oldest_nonremovable));
}
/*
@@ -2471,7 +2485,7 @@ GetSnapshotData(Snapshot snapshot)
/* apply vacuum_defer_cleanup_age */
def_vis_xid_data =
- TransactionIdRetreatedBy(xmin,
vacuum_defer_cleanup_age);
+ TransactionIdRetreatedBy(xmin,
vacuum_defer_cleanup_age, oldestfxid);
/* Check whether there's a replication slot requiring an older
xmin. */
def_vis_xid_data =
@@ -4295,32 +4309,6 @@ GlobalVisCheckRemovableXid(Relation rel, TransactionId
xid)
return GlobalVisTestIsRemovableXid(state, xid);
}
-/*
- * Convert a 32 bit transaction id into 64 bit transaction id, by assuming it
- * is within MaxTransactionId / 2 of XidFromFullTransactionId(rel).
- *
- * Be very careful about when to use this function. It can only safely be used
- * when there is a guarantee that xid is within MaxTransactionId / 2 xids of
- * rel. That e.g. can be guaranteed if the caller assures a snapshot is
- * 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.
- */
-static inline FullTransactionId
-FullXidRelativeTo(FullTransactionId rel, TransactionId xid)
-{
- TransactionId rel_xid = XidFromFullTransactionId(rel);
-
- Assert(TransactionIdIsValid(xid));
- Assert(TransactionIdIsValid(rel_xid));
-
- /* not guaranteed to find issues, but likely to catch mistakes */
- AssertTransactionIdInAllowableRange(xid);
-
- return FullTransactionIdFromU64(U64FromFullTransactionId(rel)
- +
(int32) (xid - rel_xid));
-}
-
/* ----------------------------------------------
* KnownAssignedTransactionIds sub-module
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index f5af6d3055..e094e80859 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -317,16 +317,75 @@ ReadNextTransactionId(void)
return XidFromFullTransactionId(ReadNextFullTransactionId());
}
-/* return transaction ID backed up by amount, handling wraparound correctly */
+/*
+ * Convert a 32 bit transaction id into 64 bit transaction id, by assuming it
+ * is within MaxTransactionId / 2 of XidFromFullTransactionId(rel).
+ *
+ * Be very careful about when to use this function. It can only safely be used
+ * when there is a guarantee that xid is within MaxTransactionId / 2 xids of
+ * rel. That e.g. can be guaranteed if the caller assures a snapshot is
+ * 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.
+ */
+static inline FullTransactionId
+FullXidRelativeTo(FullTransactionId rel, TransactionId xid)
+{
+ TransactionId rel_xid = XidFromFullTransactionId(rel);
+
+ Assert(TransactionIdIsValid(xid));
+ Assert(TransactionIdIsValid(rel_xid));
+
+ /* not guaranteed to find issues, but likely to catch mistakes */
+ AssertTransactionIdInAllowableRange(xid);
+
+ return FullTransactionIdFromU64(U64FromFullTransactionId(rel)
+ + (int32)(xid - rel_xid));
+}
+
+/*
+ * return transaction ID backed up by amount, handling wraparound correctly
+ *
+ * Need to be careful to prevent xid from retreating below
+ * FirstNormalTransactionId during epoch 0. This is important to prevent
+ * generating xids that cannot be converted to a FullTransactionId without
+ * wrapping around.
+ *
+ * If amount would lead to a too old xid, FirstNormalTransactionId is
+ * returned instead.
+ */
+
static inline TransactionId
-TransactionIdRetreatedBy(TransactionId xid, uint32 amount)
+TransactionIdRetreatedBy(TransactionId xid, int amount, FullTransactionId rel)
{
- xid -= amount;
+ FullTransactionId fxid;
+ uint64 fxid_i;
+ TransactionId r;
+
+ Assert(TransactionIdIsNormal(xid));
+ Assert(amount >= 0); /* relevant GUCs are stored as ints */
+ AssertTransactionIdInAllowableRange(xid);
+
+ if (amount == 0)
+ return xid;
+
+ fxid = FullXidRelativeTo(rel, xid);
+ fxid_i = U64FromFullTransactionId(fxid);
+
+ if ((fxid_i - FirstNormalTransactionId) <= amount)
+ r = FirstNormalTransactionId;
+ else
+ {
+ r = xid - amount;
+
+ while (r < FirstNormalTransactionId)
+ r--;
- while (xid < FirstNormalTransactionId)
- xid--;
+ Assert(TransactionIdIsNormal(r));
+ Assert(NormalTransactionIdPrecedes(r, xid));
+ }
- return xid;
+ return r;
}
/* return the older of the two IDs */
--
2.16.2.windows.1
