Re: Fix bug in multixact Oldest*MXactId initialization and access

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