From ccae90abdbb61b43b4cb0625f8677f1ad079c61c Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Mon, 2 Mar 2026 19:19:22 +0200 Subject: [PATCH] Fix OldestMemberMXactId and OldestVisibleMXactId array usage Commit ab355e3a88 changed how the OldestMemberMXactId array is indexed. It's no longer indexed by synthetic dummyBackendId, but with ProcNumber. The PGPROC entries for prepared xacts come after auxiliary processes in the allProcs array, which rendered the calculation for MaxOldestSlot and the indexes into the array incorrect. (The OldestVisibleMXactId array is not used for prepared xacts, and thus never accessed with ProcNumber's greater than MaxBackends, so this only affects the OldestMemberMXactId array.) As a result, a prepared xact would store its value past the end of the OldestMemberMXactId array, overflowing into the OldestVisibleMXactId array. That could cause a transaction's row lock to appear invisible to other backends, or other such visibility issues. With a very small max_connections setting, the store could even go beyond the OldestVisibleMXactId array, stomping over the first element in the BufferDescriptor array. To fix, calculate the array sizes more precisely, and introduce helper functions to calculate the array indexes correctly. Author: Yura Sokolov Reviewed-by: Sami Imseih Reviewed-by: Chao Li Discussion: https://www.postgresql.org/message-id/7acc94b0-ea82-4657-b1b0-77842cb7a60c@postgrespro.ru Backpatch-through: 17 --- src/backend/access/transam/multixact.c | 131 +++++++++++++++++-------- src/backend/access/transam/twophase.c | 7 +- src/backend/storage/lmgr/proc.c | 4 +- src/include/storage/proc.h | 1 + 4 files changed, 99 insertions(+), 44 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 90ec87d9dd6..8a5c9818ed6 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -159,12 +159,7 @@ typedef struct MultiXactStateData /* * Per-backend data starts here. We have two arrays stored in the area - * 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. + * immediately following the MultiXactStateData struct: * * OldestMemberMXactId[k] is the oldest MultiXactId each backend's current * transaction(s) could possibly be a member of, or InvalidMultiXactId @@ -176,6 +171,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 +186,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 +210,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 +221,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) +{ + int prepared_xact_idx; + + Assert(procno >= FIRST_PREPARED_XACT_PROC_NUMBER); + prepared_xact_idx = procno - FIRST_PREPARED_XACT_PROC_NUMBER; + + /* + * The first MaxBackends entries in the OldestMemberMXactId array are + * reserved for regular backends. Prepared xacts come after them. + */ + 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 +346,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 +400,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 +574,7 @@ MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly) void MultiXactIdSetOldestMember(void) { - if (!MultiXactIdIsValid(OldestMemberMXactId[MyProcNumber])) + if (!MultiXactIdIsValid(*MyOldestMemberMXactIdSlot())) { MultiXactId nextMXact; @@ -558,7 +596,7 @@ MultiXactIdSetOldestMember(void) nextMXact = MultiXactState->nextMXact; - OldestMemberMXactId[MyProcNumber] = nextMXact; + *MyOldestMemberMXactIdSlot() = nextMXact; LWLockRelease(MultiXactGenLock); @@ -586,7 +624,7 @@ MultiXactIdSetOldestMember(void) static void MultiXactIdSetOldestVisible(void) { - if (!MultiXactIdIsValid(OldestVisibleMXactId[MyProcNumber])) + if (!MultiXactIdIsValid(*MyOldestVisibleMXactIdSlot())) { MultiXactId oldestMXact; int i; @@ -594,7 +632,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 +641,7 @@ MultiXactIdSetOldestVisible(void) oldestMXact = thisoldest; } - OldestVisibleMXactId[MyProcNumber] = oldestMXact; + *MyOldestVisibleMXactIdSlot() = oldestMXact; LWLockRelease(MultiXactGenLock); @@ -1152,7 +1190,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 +1612,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 +1633,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 +1653,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 +1666,8 @@ PostPrepare_MultiXact(FullTransactionId fxid) */ LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE); - OldestMemberMXactId[dummyProcNumber] = myOldestMember; - OldestMemberMXactId[MyProcNumber] = InvalidMultiXactId; + *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = myOldestMember; + *MyOldestMemberMXactIdSlot() = InvalidMultiXactId; LWLockRelease(MultiXactGenLock); } @@ -1642,7 +1680,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 +1707,7 @@ multixact_twophase_recover(FullTransactionId fxid, uint16 info, Assert(len == sizeof(MultiXactId)); oldestMember = *((MultiXactId *) recdata); - OldestMemberMXactId[dummyProcNumber] = oldestMember; + *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = oldestMember; } /* @@ -1684,7 +1722,7 @@ multixact_twophase_postcommit(FullTransactionId fxid, uint16 info, Assert(len == sizeof(MultiXactId)); - OldestMemberMXactId[dummyProcNumber] = InvalidMultiXactId; + *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = InvalidMultiXactId; } /* @@ -1699,21 +1737,32 @@ multixact_twophase_postabort(FullTransactionId fxid, uint16 info, } /* - * Initialization of shared memory for MultiXact. We use two SLRU areas, - * thus double memory. Also, reserve space for the shared MultiXactState - * struct and the per-backend MultiXactId arrays (two of those, too). + * Initialization of shared memory for MultiXact. + * + * MultiXactSharedStateShmemSize() calculates the size of the MultiXactState + * struct, and the two per-backend MultiXactId arrays. They are carved out of + * the same allocation. MultiXactShmemSize() additionally includes the memory + * needed for the two SLRU areas. */ +static Size +MultiXactSharedStateShmemSize(void) +{ + 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 +1796,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 +1812,7 @@ MultiXactShmemInit(void) * Set up array pointers. */ OldestMemberMXactId = MultiXactState->perBackendXactIds; - OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot; + OldestVisibleMXactId = OldestMemberMXactId + NumMemberSlots; } /* @@ -2303,7 +2352,6 @@ MultiXactId GetOldestMultiXactId(void) { MultiXactId oldestMXact; - int i; /* * This is the oldest valid value among all the OldestMemberMXactId[] and @@ -2311,7 +2359,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 +2367,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;