Fix bug in multixact Oldest*MXactId initialization and access

Hi,

But proc structs reserved for two phase commit are placed after auxiliary
procs, therefore writes to OldestMemberMXactId[dummy] starts to overwrites
slots of OldestVisibleMXactId.

Then PostgreSQL 18 increased NUM_AUXILIARY_PROCS due to reserve of workers
for AIO. And it is possible to make such test postgresql.conf with so
extremely low MaxBackend so writes to OldestMemberMXactId[dummy] overwrites
first entry of BufferDescriptors, which are allocated next in shared memory.

From what I can tell, PreparedXactProcs proc numbers start at
MaxBackends + NUM_AUXILIARY_PROCS, but MaxOldestSlot is calculated as
MaxBackends + max_prepared_xacts, missing the NUM_AUXILIARY_PROCS offset.

MyProcNumber (for regular backends) is always < MaxBackends, so it's safe.
But dummyProcNumber (for prepared transactions) starts at
MaxBackends + NUM_AUXILIARY_PROCS, which exceeds MaxOldestSlot.

Using this workload:

```
psql -c "DROP TABLE IF EXISTS test;"
psql -c "CREATE TABLE IF NOT EXISTS test (id int PRIMARY KEY, data text);"
psql -c "INSERT INTO test VALUES (1, 'test data');"

for i in {1..25}; do
psql -q <<EOF
BEGIN;
SELECT * FROM test WHERE id = 1 FOR SHARE;
PREPARE TRANSACTION 'prep_$i';
EOF
echo " Created prep_$i"
done
```
I can see the assert in your patch fail here:

```
@@ -1628,8 +1631,8 @@ PostPrepare_MultiXact(FullTransactionId fxid)
*/
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);

-        OldestMemberMXactId[dummyProcNumber] = myOldestMember;
-        OldestMemberMXactId[MyProcNumber] = InvalidMultiXactId;
+        setOldest(OldestMemberMXactId, dummyProcNumber, myOldestMember);
```

Patch in attach replaces direct accesses to this arrays with inline
functions which include asserts. And changes calculation of MaxOldestSlot
to include NUM_AUXILIARY_PROCS.

This is the important part of the patch to correctly size MaxOldestSlot

```
+#define MaxOldestSlot (MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts)
```

but, I am not sure if we need all the extra asserts, as this can only
be an issue for the
dummyProcNumber, right?

Maybe we can also define a new macro:

```
#define TOTAL_PROCS MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts
```

and use it in InitProcGlobal instead of TotalProcs, as well as MaxOldestSlot?

--
Sami Imseih
Amazon Web Services

On Feb 25, 2026, at 01:35, Yura Sokolov <y.sokolov@postgrespro.ru> wrote:

Good day.

multixact.c has bug in initialization and access of OldestMemberMXactId
(and partially OldestVisibleMXactId).

Size of this arrays is defined as:

#define MaxOldestSlot (MaxBackends + max_prepared_xacts)

assuming there are only backends and prepared transactions could hold
multixacts.

This assumption is correct. And in fact there were no bug when this formula
were introduced in 2009y [1], since these arrays were indexed but synthetic
dummy `dummyBackendId` field of `GlobalTransactionData` struct.

But in 2024y [2] field `dummyBackendId` were removed and pgprocno were used
instead.

But proc structs reserved for two phase commit are placed after auxiliary
procs, therefore writes to OldestMemberMXactId[dummy] starts to overwrites
slots of OldestVisibleMXactId.

Then PostgreSQL 18 increased NUM_AUXILIARY_PROCS due to reserve of workers
for AIO. And it is possible to make such test postgresql.conf with so
extremely low MaxBackend so writes to OldestMemberMXactId[dummy] overwrites
first entry of BufferDescriptors, which are allocated next in shared memory.

Patch in attach replaces direct accesses to this arrays with inline
functions which include asserts. And changes calculation of MaxOldestSlot
to include NUM_AUXILIARY_PROCS.

Certainly, it is not clearest patch possible:
- may be you will decide to not introduce inline functions,
- or will introduce separate inline function for each array,
- or will fix slot calculation to remove aux procs from account,
- or will revert deletion of dummyBackendId.

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=cd87b6f8a5084c070c3e56b07794be8fea33647d
[2]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ab355e3a88de745607f6dd4c21f0119b5c68f2ad

--
regards
Yura Sokolov aka funny-falcon
<v00-0001-Fix-multixacts-OldestMemberMXactId-and-OldestVis.patch>

I think this is a good catch. I read through the related code paths:

In InitProcGlobal()
```
PreparedXactProcs = &procs[MaxBackends + NUM_AUXILIARY_PROCS];
```
Then in TwoPhaseShmemInit()
```
gxacts[i].pgprocno = GetNumberFromPGProc(&PreparedXactProcs[i]);
```
This shows prepared xacts are placed after MaxBackends + NUM_AUXILIARY_PROCS.

Also, in InitializeMaxBackends():
```
/* Note that this does not include "auxiliary" processes */
MaxBackends = MaxConnections + autovacuum_worker_slots +
max_worker_processes + max_wal_senders + NUM_SPECIAL_WORKER_PROCS;
```
So MaxBackends does not include NUM_AUXILIARY_PROCS.

Overall, the change looks correct to me.

One minor thought on the new helper asserts:
```
+ Assert(procno < MaxOldestSlot);
```
It may be worth also checking procno >= 0, since INVALID_PROC_NUMBER is -1.

While reading, I also noticed two stale comments related to this area. I added those fixes in 0002 (attached v2); 0001 is unchanged from the original v00.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Good day.

Chao Li and Sami Imseih, thank you for looking at.

After thinking a bit, I've decided to make sizes of arrays precise:
- OldestMemberMXactId's size remains MaxBackends + max_prepared_xacts.
Instead of changing its size, procno is now adjusted to not include
auxiliary procs.
- OldestVisibleMXactId contains only MaxBackends elemenents now.
It is used only for real backends and not prepared transactions.

All accesses are validated with asserts certainly.

I believe, index transformation in access of OldestMemberMXactId will not
cost much since all this operations are quite rare.
In the loops arrays are accessed directly since limiting loop index is enough.

--
regards
Yura Sokolov aka funny-falcon

Hi.

All accesses are validated with asserts certainly.

Maybe I am missing something, but the extra asserts and wrapper
functions being proposed seem unnecessary for this purpose.
We know the total number of procs, so we just need to make this
available to all code paths that need to index by pgprocno.

If that calculation ever changes, code like multixact.c will not
need to care.

This can be done, as mentioned earlier, by defining the total
number of procs in proc.h, which can then be used by both
multixact.c and InitProcGlobal.

```
#define TOTAL_PROCS (MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts)
```

and to calculate where the prepared transaction procs start:

```
PreparedXactProcs = &procs[TOTAL_PROCS - max_prepared_xacts];
```

What do you think?

With regards to v3, I got a compilation warning that `GetOldestVisibleMXactId`
is an unused function.

--
Sami Imseih
Amazon Web Services (AWS)

On Feb 25, 2026, at 23:16, Yura Sokolov <y.sokolov@postgrespro.ru> wrote:

Good day.

Chao Li and Sami Imseih, thank you for looking at.

After thinking a bit, I've decided to make sizes of arrays precise:
- OldestMemberMXactId's size remains MaxBackends + max_prepared_xacts.
Instead of changing its size, procno is now adjusted to not include
auxiliary procs.
- OldestVisibleMXactId contains only MaxBackends elemenents now.
It is used only for real backends and not prepared transactions.

All accesses are validated with asserts certainly.

I believe, index transformation in access of OldestMemberMXactId will not
cost much since all this operations are quite rare.
In the loops arrays are accessed directly since limiting loop index is enough.

--
regards
Yura Sokolov aka funny-falcon<v3-0001-Fix-multixacts-OldestMemberMXactId-and-OldestVisi.patch>

Actually, while I reviewing v1, I was thinking over the same way as v3, as the NUM_AUXILIARY_PROCS portions in OldestMemberMXactId and OldestVisibleMXactId arrays are not used at all. I didn’t raise the idea because that given these code are in hot paths, I wasn't sure if the complexity was worth the shared memory optimization. It’s a classic trade-off, and I’m curious to see which direction the senior committers prefer.

However, I agree that the new helper function names in v3 are a significant improvement over v1.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Good day.

Sami Imseih, I've introduced new precalculated variables:

MaxChildren = MaxBackends + NUM_AUXILIARY_PROCS;
TotalProcs = MaxChildren + max_prepared_xacts;
TotalXactProcs = MaxBackends + max_prepared_xacts;

and spread their usage through the sources.
I don't know how committers will accept this change. But they will
certainly make their own version of patch, so I don't bother much.

But I still use inline procs for access to the arrays. Asserts cost nothing
in release build. And new version of functions doesn't branch.

And you're right: looks like there were mistake in previous version so
GetOldestVisibleMXactId became unused. New version has no this mistake.

Chao Li, new access functions doen't branch because I've made separate
functions to store at slots for prepared transactions.

--

regards
Yura Sokolov aka funny-falcon

But I still use inline procs for access to the arrays. Asserts cost nothing
in release build. And new version of functions doesn't branch.

Asserts may have minimal impact for most builds, but my concern is
this is all unnecessary complexity. I mean if we guarantee that the
arrays are initialized correctly, maybe we can have an assert at
init time, why would we need asserts at run time?

--
Sami Imseih
Amazon Web Services (AWS)

26.02.2026 19:34, Sami Imseih пишет:

But I still use inline procs for access to the arrays. Asserts cost nothing
in release build. And new version of functions doesn't branch.

Asserts may have minimal impact for most builds, but my concern is
this is all unnecessary complexity. I mean if we guarantee that the
arrays are initialized correctly, maybe we can have an assert at
init time, why would we need asserts at run time?

They are future proof.

If there had been assertions from the beginning, there would not have been
a breaking change. The tests would have failed.

(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.

--
regards
Yura Sokolov aka funny-falcon

(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. I am all for
future proofing this, and I think simply centralizing the calculation
should be enough without adding complexity to the code.

Maybe others have a different opinion.

--
Sami Imseih
Amazon Web Services (AWS)

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.

--
regards
Yura Sokolov aka funny-falcon

On Feb 27, 2026, at 00:22, Yura Sokolov <y.sokolov@postgrespro.ru> wrote:

Chao Li, new access functions doen't branch because I've made separate
functions to store at slots for prepared transactions.

```
+ OldestMemberMXactId[procno - MaxChildren + MaxBackends] = mxact;
```

Wow, this is clever. It removes NUM_AUXILIARY_PROCS from procno without introducing an extra if check.

However, I am not a big fan of the new global variable names:
```
MaxChildren = MaxBackends + NUM_AUXILIARY_PROCS;
TotalProcs = MaxChildren + max_prepared_xacts;
TotalXactProcs = MaxBackends + max_prepared_xacts;
```

MaxChildren is actually the total number of backends plus auxiliary processes. Maybe something like MaxProcs would better reflect that?

TotalProcs is really the length of the PGPROC array in shared memory. Prepared transactions are not real processes; they just occupy some slots in the proc array. So perhaps a name like MaxPGProcSlots or MaxBackendAndPreparedSlots would be more accurate.

As for TotalXactProcs, maybe something like MaxBackendAndPreparedSlots would make the intent clearer.

Anyway, I am not very good at naming things. You or others may have better suggestions.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

27.02.2026 10:48, Chao Li пишет:

Anyway, I am not very good at naming things. You or others may have better suggestions.

I am bad at naming as well.
So I will not play this game.
Let committers do the way they like.

--
regards
Yura Sokolov aka funny-falcon

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

27.02.2026 13:58, Heikki Linnakangas пишет:

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 stick to separate getters and setters because they are used in 64bit xid
patch.

With 32bit TransactionId PostgreSQL code in many places rely on 32bit
atomicity. But uint64 is not so atomic on currently-mostly-ancient
architecures. Therefore there is a need to use
pg_atomic_read_u64/pg_atomic_write_u64. And without hiding them in
getters/setters code become ugly.

So, getters+setters are better suited for (possible far) future.
But I don't mind if you stick with slot references for now.

(I realized now, to be prepared for uint64 getters have to be used in
GetOldestMultiXactId and MultiXactIdSetOldestVisible as well. My bad.)

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.

With extremely low configured MaxBackends it will overflow to
BufferDescriptors array. Certainly only test configurations may use such
tiny settings.

I found the bug because I increased NUM_AUXILIARY_PROCS by bunch of other
specialized processes, so overflow to BufferDescriptors happened even with
parameters already set in tests.

--
regards
Yura Sokolov aka funny-falcon

There would no the bug. There would no this discussion.
If only array bounds were checked with asserts.

Yeah, more asserts == good, usually.

Maybe. My thoughts were that If the MaxOldestSlot is set correctly at
initialization time,
and we guarantee that by asserting its size against the total number
of procs, that should
be sufficient, and we don't need to create wrapper functions or add
assertions on every
access. But. I don't have a strong opinion either way.

Anyhow, it looks like the proposals now are aiming towards eliminating
the need to account
for auxiliary procs, and that is probably OK. It's a small bit of
memory saved, but slightly bit
more code.

With extremely low configured MaxBackends it will overflow to
BufferDescriptors array. Certainly only test configurations may use such
tiny settings.

I found the bug because I increased NUM_AUXILIARY_PROCS by bunch of other
specialized processes, so overflow to BufferDescriptors happened even with
parameters already set in tests.

It's unfortunately a bit worse. Here is a repro that shows 2 prepared
transactions
being created after a shared lock is taken on a table with 1 row. A subsequent
delete is able to complete, where we would expect it to be blocked until the
prepared transactions COMMIT or ROLLBACK.

With simply adding NUM_AUXILIARY_PROCS to MaxOldestSlot, it works as expected,
and the delete is blocked.

```
-#define MaxOldestSlot  (MaxBackends + max_prepared_xacts)
+#define MaxOldestSlot  (MaxBackends + NUM_AUXILIARY_PROCS + max_prepared_xacts)
 ```

I also tried with Heikki's proposal and the test succeeded. I did not
review the patch thoroughly
yet, but I think this test case should be added.

The test does require 2 prepared transactions to exercise
MultiXactIdSetOldestVisible(),
which then results in reading garbage values from the array and
determining incorrect
visibility of the row.

--
Sami Imseih
Amazon Web Services (AWS)

Here is a repro that shows

sorry, reattaching another version of the repro cleaned up
from remnants of an earlier renaming of the test.

--
Sami Imseih
Amazon Web Services (AWS)

27.02.2026 21:51, Sami Imseih пишет:

It's unfortunately a bit worse. Here is a repro that shows 2 prepared
transactions
being created after a shared lock is taken on a table with 1 row. A subsequent
delete is able to complete, where we would expect it to be blocked until the
prepared transactions COMMIT or ROLLBACK.

OH MY GOD!!!!
I saw very similar behavior in dump of production WAL logs from our client.
Main issue were in other subsystem in that case, that is why we didn't dig
deeper.
But I clearly remembered: "Ouch, it is strange, why this row were deleted
being locked for foreign key?".

Great catch!!!!
My deep respect to you, Sami Imseih!!!!

With simply adding NUM_AUXILIARY_PROCS to MaxOldestSlot, it works as expected,
and the delete is blocked.

--
regards
Yura Sokolov aka funny-falcon

(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

28.02.2026 12:22, Heikki Linnakangas пишет:

New version attached.

LGTM

--
regards
Yura Sokolov