From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Yura Sokolov <y.sokolov@postgrespro.ru>
To: Sami Imseih <samimseih@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Chao Li <li.evan.chao@gmail.com>
Subject: Re: Fix bug in multixact Oldest*MXactId initialization and access
Date: Fri, 27 Feb 2026 12:58:21 +0200
Message-ID: <366b6f95-613d-4ef8-82e4-bb2fd06d9c4d@iki.fi> (raw)
In-Reply-To: <3d7a2207-0e61-4d95-bc00-e5248956a32b@postgrespro.ru>
References: <7acc94b0-ea82-4657-b1b0-77842cb7a60c@postgrespro.ru>
<93F0BD0A-0B8B-4592-9D4B-53F405A86F6A@gmail.com>
<c3eaaac1-d244-4e4d-b5d7-06c284108eac@postgrespro.ru>
<C991344D-F38A-4EEC-903A-72B52FF887FA@gmail.com>
<bf2951b4-687b-4bd3-a072-46deb463f177@postgrespro.ru>
<CAA5RZ0t8iGnqO3JcAZP1VEjCspcnZtQ0GFFVGLNLAsFqfHqRsw@mail.gmail.com>
<ebeb0db1-0eb2-4439-a6c8-1d96b60b25ad@postgrespro.ru>
<CAA5RZ0s+rr=DVkFnMwupgnCAvPjJkjQ3jr5=7X_=Sb5Mr02-Mg@mail.gmail.com>
<3d7a2207-0e61-4d95-bc00-e5248956a32b@postgrespro.ru>
On 27/02/2026 07:00, Yura Sokolov wrote:
> 26.02.2026 23:44, Sami Imseih пишет:
>>> (Sorry, I've used Google Translate to write this sentence).
>>>
>>> When you write assert, you protect yourself from shooting your leg far in
>>> the future. Believe me.
>>
>> The issue to me seems that we a few code paths that rely on the
>> total proc calculation in several places, and changing one and
>> forgetting to change another is what broke things.
>
> If there were asserts, then tests would have failed.
> Period.
>
> There would no the bug. There would no this discussion.
> If only array bounds were checked with asserts.
Yeah, more asserts == good, usually.
Here's my version of this. It's the same basic idea, some minor changes:
- Instead of the MaxChildren, TotalProcs, TotalXactProcs variables, I
added FIRST_PREPARED_XACT_PROC_NUMBER. Rationale: The variables still
required you to know the layout of the PGPROCs, i.e. you still had to
know that aux processes come after regular backends and dummy pgprocs
after aux processes. An FIRST_PREPARED_XACT_PROC_NUMBER is more explicit
in the places where it's needed.
I have been thinking of adding variables like that anyway, because
the current calculations with MaxBackends et al. are just so confusing.
So I'm vaguely in favor of doing something like that in the future. But
I think FIRST_PREPARED_XACT_PROC_NUMBER is more clear for this patch,
and those variable names as done here (MaxBackends, MaxChildren,
Totalprocs and TotalXactProcs) were still super confusing.
- I added a slightly different the set of Getter/Setter functions:
MyOldestMemberMXactIdSlot(),
PreparedXactOldestMemberMXactIdSlot(ProcNumber), and
MyOldestVisibleMXactIdSlot(). I think this is slightly less error-prone,
making it harder to pass proc number to wrong function (although the
assertions would catch such misuses pretty quick anyway). And these
functions return a pointer to the slot, so we don't need separate getter
and setter functions. That's a matter of taste, having a little less
code looks nicer to me.
What do you think?
I've been pondering what kind of issues this bug could cause. Because
the OldestVisibleMXactId array is allocated just after the
OldestMemberMXactId array, you don't get a buffer overflow, but a
prepared xact can overwrite a backend's value in the
OldestVisibleMXactId array. That can surely cause trouble later, but not
sure what exactly, and it seems pretty hard to write a repro script for it.
- Heikki
Attachments:0001-Fix-multixacts-OldestMemberMXactId-and-OldestVisible.patch (text/x-patch, 14.1K) download
From 1bc2b558710aab2ee2c94c319b27587907cc3695 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 27 Feb 2026 12:30:24 +0200
Subject: [PATCH 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 <y.sokolov@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/7acc94b0-ea82-4657-b1b0-77842cb7a60c@postgrespro.ru
---
src/backend/access/transam/multixact.c | 106 +++++++++++++++++--------
src/backend/access/transam/twophase.c | 7 +-
src/backend/storage/lmgr/proc.c | 4 +-
src/include/storage/proc.h | 1 +
4 files changed, 79 insertions(+), 39 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.cindex 90ec87d9dd6..bc45edb4c47 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,28 @@ static MultiXactId *OldestMemberMXactId;
static MultiXactId *OldestVisibleMXactId;
+static inline MultiXactId *+MyOldestMemberMXactIdSlot(void)+{+ Assert(MyProcNumber >= 0 && MyProcNumber < MaxBackends);+ return &OldestMemberMXactId[MyProcNumber];+}++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];+}++static inline MultiXactId *+MyOldestVisibleMXactIdSlot(void)+{+ Assert(MyProcNumber >= 0 && MyProcNumber < NumVisibleSlots);+ return &OldestVisibleMXactId[MyProcNumber];+}+
/*
* Definitions for the backend-local MultiXactId cache.
*
@@ -308,7 +334,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 +388,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 +562,7 @@ MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly)
void
MultiXactIdSetOldestMember(void)
{
- if (!MultiXactIdIsValid(OldestMemberMXactId[MyProcNumber]))+ if (!MultiXactIdIsValid(*MyOldestMemberMXactIdSlot()))
{
MultiXactId nextMXact;
@@ -558,7 +584,7 @@ MultiXactIdSetOldestMember(void)
nextMXact = MultiXactState->nextMXact;
- OldestMemberMXactId[MyProcNumber] = nextMXact;+ *MyOldestMemberMXactIdSlot() = nextMXact;
LWLockRelease(MultiXactGenLock);
@@ -586,7 +612,7 @@ MultiXactIdSetOldestMember(void)
static void
MultiXactIdSetOldestVisible(void)
{
- if (!MultiXactIdIsValid(OldestVisibleMXactId[MyProcNumber]))+ if (!MultiXactIdIsValid(*MyOldestVisibleMXactIdSlot()))
{
MultiXactId oldestMXact;
int i;
@@ -594,7 +620,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 +629,7 @@ MultiXactIdSetOldestVisible(void)
oldestMXact = thisoldest;
}
- OldestVisibleMXactId[MyProcNumber] = oldestMXact;+ *MyOldestVisibleMXactIdSlot() = oldestMXact;
LWLockRelease(MultiXactGenLock);
@@ -1152,7 +1178,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 +1600,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 +1621,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 +1641,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 +1654,8 @@ PostPrepare_MultiXact(FullTransactionId fxid)
*/
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);
- OldestMemberMXactId[dummyProcNumber] = myOldestMember;- OldestMemberMXactId[MyProcNumber] = InvalidMultiXactId;+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = myOldestMember;+ *MyOldestMemberMXactIdSlot() = InvalidMultiXactId;
LWLockRelease(MultiXactGenLock);
}
@@ -1642,7 +1668,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 +1695,7 @@ multixact_twophase_recover(FullTransactionId fxid, uint16 info,
Assert(len == sizeof(MultiXactId));
oldestMember = *((MultiXactId *) recdata);
- OldestMemberMXactId[dummyProcNumber] = oldestMember;+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = oldestMember;
}
/*
@@ -1684,7 +1710,7 @@ multixact_twophase_postcommit(FullTransactionId fxid, uint16 info,
Assert(len == sizeof(MultiXactId));
- OldestMemberMXactId[dummyProcNumber] = InvalidMultiXactId;+ *PreparedXactOldestMemberMXactIdSlot(dummyProcNumber) = InvalidMultiXactId;
}
/*
@@ -1703,17 +1729,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 +1781,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 +1797,7 @@ MultiXactShmemInit(void)
* Set up array pointers.
*/
OldestMemberMXactId = MultiXactState->perBackendXactIds;
- OldestVisibleMXactId = OldestMemberMXactId + MaxOldestSlot;+ OldestVisibleMXactId = OldestMemberMXactId + NumMemberSlots;
}
/*
@@ -2303,7 +2337,6 @@ MultiXactId
GetOldestMultiXactId(void)
{
MultiXactId oldestMXact;
- int i;
/*
* This is the oldest valid value among all the OldestMemberMXactId[] and
@@ -2311,7 +2344,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 +2352,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.cindex 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.cindex 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.hindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, y.sokolov@postgrespro.ru, samimseih@gmail.com, pgsql-hackers@lists.postgresql.org, andres@anarazel.de, li.evan.chao@gmail.com
Subject: Re: Fix bug in multixact Oldest*MXactId initialization and access
In-Reply-To: <366b6f95-613d-4ef8-82e4-bb2fd06d9c4d@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox