| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru> |
| Cc: | "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Andres Freund <andres(at)anarazel(dot)de> |
| Subject: | Re: Fix bug in multixact Oldest*MXactId initialization and access |
| Date: | 2026-02-25 01:44:22 |
| Message-ID: | 93F0BD0A-0B8B-4592-9D4B-53F405A86F6A@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Feb 25, 2026, at 01:35, Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)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/
| Attachment | Content-Type | Size |
|---|---|---|
| v2-0001-Fix-multixacts-OldestMemberMXactId-and-OldestVisi.patch | application/octet-stream | 7.2 KB |
| v2-0002-Fix-stale-comments-about-prepared-xact-proc-numbe.patch | application/octet-stream | 2.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andrew Dunstan | 2026-02-25 02:06:57 | Re: pgsql: libpq: Grease the protocol by default |
| Previous Message | Tom Lane | 2026-02-25 01:30:29 | Re: pgsql: libpq: Grease the protocol by default |