(added back pgsql-hackers, I think you replied in private by accident)
On 28/02/2026 00:07, Sami Imseih wrote:
1/
+MyOldestMemberMXactIdSlot(void)
+{
+ Assert(MyProcNumber >= 0 && MyProcNumber < MaxBackends);
It would be better to use NumVisibleSlots instead of MaxBackends in
the second assert condition.
MaxBackends makes the assertion more strict. It verifies that we use one
of the slots reserved for regular backends, not prepared xacts. I'll add
a comment on that.
2/
+static inline MultiXactId *
+PreparedXactOldestMemberMXactIdSlot(ProcNumber procno)
+{
+ Assert(procno >= FIRST_PREPARED_XACT_PROC_NUMBER);
+ Assert(procno - FIRST_PREPARED_XACT_PROC_NUMBER <
+ NumMemberSlots);
+ return &OldestMemberMXactId[procno -
+ FIRST_PREPARED_XACT_PROC_NUMBER];
+}
given
+#define FIRST_PREPARED_XACT_PROC_NUMBER \
+ (MaxBackends + NUM_AUXILIARY_PROCS)
let's say, MaxBackends = 100 and NUM_AUXILIARY_PROCS = 38, the
first prepared transaction procno will be 138. The current math
will return an index of 0 (138 - 138 = 0), but this corrupts
backend slot 0.
Oof, you're right.
Should we not account for NumVisibleSlots to skip
over the regular backend slots?
```
return &OldestMemberMXactId[NumVisibleSlots + (procno -
FIRST_PREPARED_XACT_PROC_NUMBER)];
```
That's not quite right either. NumVisibleSlots has nothing to do with
the OldestMemberMXactId array, MaxBackends is the right offset here.
NumVisibleSlots == MaxBackends, but that's just a coincidence.
New version attached.
- Heikki
From 47b97f62aecf3a6fe62cd512efbe66e7a69c69f8 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Sat, 28 Feb 2026 11:17:17 +0200
Subject: [PATCH v2 1/1] Fix multixacts OldestMemberMXactId and
OldestVisibleMXactId usage
Due to [1], OldestMemberMXactId is no longer accessed by synthetic
dummyBackendId, but rather with pgprocno. Procs for prepared xacts are
placed after auxiliary procs, therefore calculation for MaxOldestSlot
became invalid.
On the other hand, OldestVisibleMXactId is used only for real backends,
and so never accessed at index greater than MaxBackends.
Lets separate size calculation for arrays and use converted index to
store oldest member mxact for prepared transactions.
[1] ab355e3a88de745 "Redefine backend ID to be an index into the proc array"
Author: Yura Sokolov <[email protected]>
Reviewed-by: Sami Imseih <[email protected]>
Discussion: https://www.postgresql.org/message-id/[email protected]
---
src/backend/access/transam/multixact.c | 119 ++++++++++++++++++-------
src/backend/access/transam/twophase.c | 7 +-
src/backend/storage/lmgr/proc.c | 4 +-
src/include/storage/proc.h | 1 +
4 files changed, 92 insertions(+), 39 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 90ec87d9dd6..5dd0ddf6f5a 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -162,10 +162,6 @@ typedef struct MultiXactStateData
* immediately following the MultiXactStateData struct. Each is indexed by
* ProcNumber.
*
- * In both arrays, there's a slot for all normal backends
- * (0..MaxBackends-1) followed by a slot for max_prepared_xacts prepared
- * transactions.
- *
* OldestMemberMXactId[k] is the oldest MultiXactId each backend's current
* transaction(s) could possibly be a member of, or InvalidMultiXactId
* when the backend has no live transaction that could possibly be a
@@ -176,6 +172,10 @@ typedef struct MultiXactStateData
* member of a MultiXact, and that MultiXact would have to be created
* during or after the lock acquisition.)
*
+ * In the OldestMemberMXactId array, there's a slot for all normal
+ * backends (0..MaxBackends-1) followed by a slot for max_prepared_xacts
+ * prepared transactions.
+ *
* OldestVisibleMXactId[k] is the oldest MultiXactId each backend's
* current transaction(s) think is potentially live, or InvalidMultiXactId
* when not in a transaction or not in a transaction that's paid any
@@ -187,6 +187,9 @@ typedef struct MultiXactStateData
* than its own OldestVisibleMXactId[] setting; this is necessary because
* the relevant SLRU data can be concurrently truncated away.
*
+ * In the OldestVisibleMXactId array, there's a slot for all normal
+ * backends (0..MaxBackends-1) only. No slots for prepared transactions.
+ *
* The oldest valid value among all of the OldestMemberMXactId[] and
* OldestVisibleMXactId[] entries is considered by vacuum as the earliest
* possible value still having any live member transaction -- OldestMxact.
@@ -208,9 +211,10 @@ typedef struct MultiXactStateData
} MultiXactStateData;
/*
- * Size of OldestMemberMXactId and OldestVisibleMXactId arrays.
+ * Sizes of OldestMemberMXactId and OldestVisibleMXactId arrays.
*/
-#define MaxOldestSlot (MaxBackends + max_prepared_xacts)
+#define NumMemberSlots (MaxBackends + max_prepared_xacts)
+#define NumVisibleSlots MaxBackends
/* Pointers to the state data in shared memory */
static MultiXactStateData *MultiXactState;
@@ -218,6 +222,41 @@ static MultiXactId *OldestMemberMXactId;
static MultiXactId *OldestVisibleMXactId;
+static inline MultiXactId *
+MyOldestMemberMXactIdSlot(void)
+{
+ /*
+ * The first MaxBackends entries in the OldestMemberMXactId array are
+ * reserved for regular backends. MyProcNumber should index into one of
+ * them.
+ */
+ Assert(MyProcNumber >= 0 && MyProcNumber < MaxBackends);
+ return &OldestMemberMXactId[MyProcNumber];
+}
+
+static inline MultiXactId *
+PreparedXactOldestMemberMXactIdSlot(ProcNumber procno)
+{
+ /*
+ * The entries with indexes >= MaxBackends in the OldestMemberMXactId
+ * array are reserved for regular backends.
+ */
+ int prepared_xact_idx;
+
+ Assert(procno >= FIRST_PREPARED_XACT_PROC_NUMBER);
+ prepared_xact_idx = procno - FIRST_PREPARED_XACT_PROC_NUMBER;
+
+ Assert(MaxBackends + prepared_xact_idx < NumMemberSlots);
+ return &OldestMemberMXactId[MaxBackends + prepared_xact_idx];
+}
+
+static inline MultiXactId *
+MyOldestVisibleMXactIdSlot(void)
+{
+ Assert(MyProcNumber >= 0 && MyProcNumber < NumVisibleSlots);
+ return &OldestVisibleMXactId[MyProcNumber];
+}
+
/*
* Definitions for the backend-local MultiXactId cache.
*
@@ -308,7 +347,7 @@ MultiXactIdCreate(TransactionId xid1, MultiXactStatus status1,
Assert(!TransactionIdEquals(xid1, xid2) || (status1 != status2));
/* MultiXactIdSetOldestMember() must have been called already. */
- Assert(MultiXactIdIsValid(OldestMemberMXactId[MyProcNumber]));
+ Assert(MultiXactIdIsValid(*MyOldestMemberMXactIdSlot()));
/*
* Note: unlike MultiXactIdExpand, we don't bother to check that both XIDs
@@ -362,7 +401,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
Assert(TransactionIdIsValid(xid));
/* MultiXactIdSetOldestMember() must have been called already. */
- Assert(MultiXactIdIsValid(OldestMemberMXactId[MyProcNumber]));
+ Assert(MultiXactIdIsValid(*MyOldestMemberMXactIdSlot()));
debug_elog5(DEBUG2, "Expand: received multi %u, xid %u status %s",
multi, xid, mxstatus_to_string(status));
@@ -536,7 +575,7 @@ MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly)
void
MultiXactIdSetOldestMember(void)
{
- if (!MultiXactIdIsValid(OldestMemberMXactId[MyProcNumber]))
+ if (!MultiXactIdIsValid(*MyOldestMemberMXactIdSlot()))
{
MultiXactId nextMXact;
@@ -558,7 +597,7 @@ MultiXactIdSetOldestMember(void)
nextMXact = MultiXactState->nextMXact;
- OldestMemberMXactId[MyProcNumber] = nextMXact;
+ *MyOldestMemberMXactIdSlot() = nextMXact;
LWLockRelease(MultiXactGenLock);
@@ -586,7 +625,7 @@ MultiXactIdSetOldestMember(void)
static void
MultiXactIdSetOldestVisible(void)
{
- if (!MultiXactIdIsValid(OldestVisibleMXactId[MyProcNumber]))
+ if (!MultiXactIdIsValid(*MyOldestVisibleMXactIdSlot()))
{
MultiXactId oldestMXact;
int i;
@@ -594,7 +633,7 @@ MultiXactIdSetOldestVisible(void)
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);
oldestMXact = MultiXactState->nextMXact;
- for (i = 0; i < MaxOldestSlot; i++)
+ for (i = 0; i < NumMemberSlots; i++)
{
MultiXactId thisoldest = OldestMemberMXactId[i];
@@ -603,7 +642,7 @@ MultiXactIdSetOldestVisible(void)
oldestMXact = thisoldest;
}
- OldestVisibleMXactId[MyProcNumber] = oldestMXact;
+ *MyOldestVisibleMXactIdSlot() = oldestMXact;
LWLockRelease(MultiXactGenLock);
@@ -1152,7 +1191,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
* multi. It cannot possibly still be running.
*/
if (isLockOnly &&
- MultiXactIdPrecedes(multi, OldestVisibleMXactId[MyProcNumber]))
+ MultiXactIdPrecedes(multi, *MyOldestVisibleMXactIdSlot()))
{
debug_elog2(DEBUG2, "GetMembers: a locker-only multi is too old");
*members = NULL;
@@ -1574,8 +1613,8 @@ AtEOXact_MultiXact(void)
* We assume that storing a MultiXactId is atomic and so we need not take
* MultiXactGenLock to do this.
*/
- OldestMemberMXactId[MyProcNumber] = InvalidMultiXactId;
- OldestVisibleMXactId[MyProcNumber] = InvalidMultiXactId;
+ *MyOldestMemberMXactIdSlot() = InvalidMultiXactId;
+ *MyOldestVisibleMXactIdSlot() = InvalidMultiXactId;
/*
* Discard the local MultiXactId cache. Since MXactContext was created as
@@ -1595,7 +1634,7 @@ AtEOXact_MultiXact(void)
void
AtPrepare_MultiXact(void)
{
- MultiXactId myOldestMember = OldestMemberMXactId[MyProcNumber];
+ MultiXactId myOldestMember = *MyOldestMemberMXactIdSlot();
if (MultiXactIdIsValid(myOldestMember))
RegisterTwoPhaseRecord(TWOPHASE_RM_MULTIXACT_ID, 0,
@@ -1615,7 +1654,7 @@ PostPrepare_MultiXact(FullTransactionId fxid)
* Transfer our OldestMemberMXactId value to the slot reserved for the
* prepared transaction.
*/
- myOldestMember = OldestMemberMXactId[MyProcNumber];
+ myOldestMember = *MyOldestMemberMXactIdSlot();
if (MultiXactIdIsValid(myOldestMember))
{
ProcNumber dummyProcNumber = TwoPhaseGetDummyProcNumber(fxid, false);
@@ -1628,8 +1667,8 @@ PostPrepare_MultiXact(FullTransactionId fxid)
*/
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);
- OldestMemberMXactId[dummyProcNumber] = myOldestMember;
- OldestMemberMXactId[MyProcNumber] = InvalidMultiXactId;
+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = myOldestMember;
+ *MyOldestMemberMXactIdSlot() = InvalidMultiXactId;
LWLockRelease(MultiXactGenLock);
}
@@ -1642,7 +1681,7 @@ PostPrepare_MultiXact(FullTransactionId fxid)
* We assume that storing a MultiXactId is atomic and so we need not take
* MultiXactGenLock to do this.
*/
- OldestVisibleMXactId[MyProcNumber] = InvalidMultiXactId;
+ *MyOldestVisibleMXactIdSlot() = InvalidMultiXactId;
/*
* Discard the local MultiXactId cache like in AtEOXact_MultiXact.
@@ -1669,7 +1708,7 @@ multixact_twophase_recover(FullTransactionId fxid, uint16 info,
Assert(len == sizeof(MultiXactId));
oldestMember = *((MultiXactId *) recdata);
- OldestMemberMXactId[dummyProcNumber] = oldestMember;
+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = oldestMember;
}
/*
@@ -1684,7 +1723,7 @@ multixact_twophase_postcommit(FullTransactionId fxid, uint16 info,
Assert(len == sizeof(MultiXactId));
- OldestMemberMXactId[dummyProcNumber] = InvalidMultiXactId;
+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = InvalidMultiXactId;
}
/*
@@ -1703,17 +1742,25 @@ multixact_twophase_postabort(FullTransactionId fxid, uint16 info,
* thus double memory. Also, reserve space for the shared MultiXactState
* struct and the per-backend MultiXactId arrays (two of those, too).
*/
+static Size
+MultiXactSharedStateShmemSize()
+{
+ Size size;
+
+ size = offsetof(MultiXactStateData, perBackendXactIds);
+ size = add_size(size,
+ mul_size(sizeof(MultiXactId), NumMemberSlots));
+ size = add_size(size,
+ mul_size(sizeof(MultiXactId), NumVisibleSlots));
+ return size;
+}
+
Size
MultiXactShmemSize(void)
{
Size size;
- /* We need 2*MaxOldestSlot perBackendXactIds[] entries */
-#define SHARED_MULTIXACT_STATE_SIZE \
- add_size(offsetof(MultiXactStateData, perBackendXactIds), \
- mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot))
-
- size = SHARED_MULTIXACT_STATE_SIZE;
+ size = MultiXactSharedStateShmemSize();
size = add_size(size, SimpleLruShmemSize(multixact_offset_buffers, 0));
size = add_size(size, SimpleLruShmemSize(multixact_member_buffers, 0));
@@ -1747,14 +1794,14 @@ MultiXactShmemInit(void)
/* Initialize our shared state struct */
MultiXactState = ShmemInitStruct("Shared MultiXact State",
- SHARED_MULTIXACT_STATE_SIZE,
+ MultiXactSharedStateShmemSize(),
&found);
if (!IsUnderPostmaster)
{
Assert(!found);
/* Make sure we zero out the per-backend state */
- MemSet(MultiXactState, 0, SHARED_MULTIXACT_STATE_SIZE);
+ MemSet(MultiXactState, 0, MultiXactSharedStateShmemSize());
}
else
Assert(found);
@@ -1763,7 +1810,7 @@ MultiXactShmemInit(void)
* Set up array pointers.
*/
OldestMemberMXactId = MultiXactState->perBackendXactIds;
- OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot;
+ OldestVisibleMXactId = OldestMemberMXactId + NumMemberSlots;
}
/*
@@ -2303,7 +2350,6 @@ MultiXactId
GetOldestMultiXactId(void)
{
MultiXactId oldestMXact;
- int i;
/*
* This is the oldest valid value among all the OldestMemberMXactId[] and
@@ -2311,7 +2357,7 @@ GetOldestMultiXactId(void)
*/
LWLockAcquire(MultiXactGenLock, LW_SHARED);
oldestMXact = MultiXactState->nextMXact;
- for (i = 0; i < MaxOldestSlot; i++)
+ for (int i = 0; i < NumMemberSlots; i++)
{
MultiXactId thisoldest;
@@ -2319,6 +2365,11 @@ GetOldestMultiXactId(void)
if (MultiXactIdIsValid(thisoldest) &&
MultiXactIdPrecedes(thisoldest, oldestMXact))
oldestMXact = thisoldest;
+ }
+ for (int i = 0; i < NumVisibleSlots; i++)
+ {
+ MultiXactId thisoldest;
+
thisoldest = OldestVisibleMXactId[i];
if (MultiXactIdIsValid(thisoldest) &&
MultiXactIdPrecedes(thisoldest, oldestMXact))
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index e4340b59640..5f65fa7b80f 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -900,9 +900,10 @@ TwoPhaseGetXidByVirtualXID(VirtualTransactionId vxid,
* Get the dummy proc number for prepared transaction
*
* Dummy proc numbers are similar to proc numbers of real backends. They
- * start at MaxBackends, and are unique across all currently active real
- * backends and prepared transactions. If lock_held is set to true,
- * TwoPhaseStateLock will not be taken, so the caller had better hold it.
+ * start at FIRST_PREPARED_XACT_PROC_NUMBER, and are unique across all
+ * currently active real backends and prepared transactions. If lock_held is
+ * set to true, TwoPhaseStateLock will not be taken, so the caller had better
+ * hold it.
*/
ProcNumber
TwoPhaseGetDummyProcNumber(FullTransactionId fxid, bool lock_held)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 771b006b522..ccf9de0e67c 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -304,7 +304,7 @@ InitProcGlobal(void)
* dummy PGPROCs don't need these though - they're never associated
* with a real process
*/
- if (i < MaxBackends + NUM_AUXILIARY_PROCS)
+ if (i < FIRST_PREPARED_XACT_PROC_NUMBER)
{
proc->sem = PGSemaphoreCreate();
InitSharedLatch(&(proc->procLatch));
@@ -369,7 +369,7 @@ InitProcGlobal(void)
* processes and prepared transactions.
*/
AuxiliaryProcs = &procs[MaxBackends];
- PreparedXactProcs = &procs[MaxBackends + NUM_AUXILIARY_PROCS];
+ PreparedXactProcs = &procs[FIRST_PREPARED_XACT_PROC_NUMBER];
}
/*
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index a8d2e7db1a1..3f89450c216 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -532,6 +532,7 @@ extern PGDLLIMPORT PGPROC *PreparedXactProcs;
#define MAX_IO_WORKERS 32
#define NUM_AUXILIARY_PROCS (6 + MAX_IO_WORKERS)
+#define FIRST_PREPARED_XACT_PROC_NUMBER (MaxBackends + NUM_AUXILIARY_PROCS)
/* configurable options */
extern PGDLLIMPORT int DeadlockTimeout;
--
2.47.3
