Message-ID: <6c298bc4-7029-4c1d-bf16-3e094842ce32@iki.fi>
Date: Fri, 21 Nov 2025 15:56:33 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: POC: make mxidoff 64 bits
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Maxim Orlov <orlovmg@gmail.com>, Alvaro Herrera
 <alvherre@alvh.no-ip.org>, Alexander Korotkov <aekorotkov@gmail.com>,
 wenhui qiu <qiuwenhuifx@gmail.com>,
 Postgres hackers <pgsql-hackers@lists.postgresql.org>
References: 
 <CACG=ezaWg7_nt-8ey4aKv2w9LcuLthHknwCawmBgEeTnJrJTcw@mail.gmail.com>
 <CACG=ezajc_Pcqmy6fcq-N8+LzCRMzOzJzez2_BgHEu-6RVJtKQ@mail.gmail.com>
 <4535f3aa-3220-4760-b1f5-2bc91f248e03@iki.fi>
 <CACG=ezbKwypBp=14q9+hMQApus3=1hKxJ9x1+KinUhtT48570Q@mail.gmail.com>
 <2bc58592-9d74-4af0-bdd1-1a88e8683f7c@iki.fi>
 <CACG=ezZwdvsijzuXE3hex3xHcoz75EQYBXRTsQJVwbo5J5sS3g@mail.gmail.com>
 <CACG=ezbs912S58=uR17b4w8uuWv1=OcCRaTW_OWdFm4+tXZA6w@mail.gmail.com>
 <CAGjGUA+BfcWyccNN4=tHsW_E-koRxbg8h8ut6hjvPsHMgmek6w@mail.gmail.com>
 <CACG=ezYbYO_KHWdeDedbDcY0tOS0JfaqBxG3=bG5+DdsDK4MpQ@mail.gmail.com>
 <CACG=ezYpZRPwoRCz_h3Qerd3XJNdpTHCpwGbZphNdy26tA4_qQ@mail.gmail.com>
 <36531c0e-292c-409d-bbc7-a252cf6e910a@iki.fi>
 <54aa8f65-f0e4-4464-b543-e0399c1cab1e@iki.fi>
 <CACG=ezYUJSvnuxntkURNWo_1vZ+AtmcQfqd_h6WgDzGaudfw+Q@mail.gmail.com>
 <4a9dda70-0af7-41a4-9636-b168f2fc48ef@iki.fi>
 <46cc45e9-fddd-44bc-bcb3-96889aafd921@iki.fi>
 <CAExHW5tUEkiQrvm9hgccjKUNkWBnJ5_HDUrAwiHBTxu+Vuj29Q@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CAExHW5tUEkiQrvm9hgccjKUNkWBnJ5_HDUrAwiHBTxu+Vuj29Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/6c298bc4-7029-4c1d-bf16-3e094842ce32%40iki.fi>
Precedence: bulk

On 21/11/2025 14:15, Ashutosh Bapat wrote:
> On Mon, Nov 17, 2025 at 10:05=E2=80=AFPM Heikki Linnakangas <hlinnaka@i=
ki.fi> wrote:
>>
>> Ashutosh, you were interested in reviewing this earlier. Would you hav=
e
>> a chance to review this now, before I commit it?
>=20
> 0002 seems to be fine. It's just moving code from one file to another.
> However, the name multixact_internals.h seems to be misusing term
> "internal".  I would expect an "internal.h" to expose things for use
> within the module and not "externally" in other modules. But this
> isn't the first time, buf_internal.h, toast_internal.h
> bgworker_internal.h and so on have their own meaning of what
> "internal" means to them. But it might be better to use something more
> meaningful than "internal" in this case. The file seems to contain
> declarations related to how pg_multixact SLRU is structured. To that
> effect multixact_slru.h or multixact_layout.h may be more appropriate.

Yeah, I went with multixact_internal.h because of the precedence. It's=20
not great, but IMHO it's better to be consistent than invent a new=20
naming scheme.

> There are two offsets that that file deals with offset within
> pg_multixact/offset, MultiXactOffset and member offset (flag offset
> and xid offset) within pg_multixact/members. It's quite easy to get
> confused between those when reading that code.

Agreed, those are confusing. I'll think about that a little more.

> The reason why this eliminates the need for wraparound is mentioned
> somewhere in GetNewMultiXactId(), but probably it should be mentioned
> at a more prominent place and also in the commit message. I expected
> it to be in the prologue of GetNewMultiXactId(), and then a reference
> to prologue from where the comment is right now.
>=20
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> errmsg("MultiXact members would wrap around")));
>=20
>   If a server ever reaches this point, there is no way but to create
> another cluster, if the applications requires multi-xact ids?

Pretty much. You can also vacuum freeze everything, so that no multixids=20
are in use, and then use pg_resetwal to reset nextOffset to a lower value=
.

That obviously sounds bad, but this is a "can't happen" situation. For=20
comparison, we don't provide any better way to recover from running out=20
of LSNs either.

> We should also provide this as an errhint().
I think no. You cannot run into this "organically" by just running the=20
system. If you do manage to hit it, it's a sign of some other trouble,=20
and I don't want to guess what that might be, or what might be the=20
appropriate way to recover.

> In ExtendMultiXactMember() the last comment mentions a wraparound
> /*
> * Advance to next page, taking care to properly handle the wraparound
> * case. OK if nmembers goes negative.
> */
> I thought this wraparound is about offset wraparound, which can not
> happen now. Since you have left the comment intact, it's something
> else. Is the wraparound of offset within the page? Maybe requires a
> bit more clarification?

It was indeed about offset wraparound. I'll remove it.

> PerformMembersTruncation(MultiXactOffset oldestOffset, MultiXactOffset
> newOldestOffset)
> {
> ... snip ...
> - segment +=3D 1;
> - }
> + SimpleLruTruncate(MultiXactMemberCtl,
> + MXOffsetToMemberPage(newOldestOffset));
> }
>=20
> Most of the callers of SimpleLruTruncate() call it directly. Why do we
> want to keep this static wrapper? PerformOffsetsTruncation() has a
> comment which seems to need the wrapper. But
> PerformMembersTruncation() doesn't even have that.

Hmm, yeah those wrappers are a bit vestigial now. I'm inclined to keep=20
them, because as you said, PerformOffsetsTruncation() provides a place=20
for the comment. And given that, it seems best to keep=20
PerformMembersTruncation(), for symmetry.

> MultiXactState->oldestMultiXactId =3D newOldestMulti;
> MultiXactState->oldestMultiXactDB =3D newOldestMultiDB;
> + MultiXactState->oldestOffset =3D newOldestOffset;
> LWLockRelease(MultiXactGenLock);
>=20
> Is this something we are missing in the current code? I can not
> understand the connection between this change and the fact that offset
> wraparound is not possible with wider multi xact offsets. Maybe I
> missed some previous discussion.

Good question. At first I intended to extract that to a separate commit,=20
before the main patch, because it seems like a nice improvement: We have=20
just calculated 'oldestOffset', so why not update the value in shared=20
memory while we have it? But looking closer, I'm not sure if it'd be=20
sane without the 64-bit offsets. Currently, oldestOffset is only updated=20
by SetOffsetVacuumLimit(), which also updates offsetStopLimit. We could=20
get into a state where oldestOffset is set, but offsetStopLimit is not.=20
With 64-bit offsets that's no longer a concern because it removes=20
offsetStopLimit altogether.

> I have reviewed patch 0002 and multxact.c changes in 0003. So far I
> have only these comments. I will review the pg_upgrade.c changes next.

Thanks for the review so far!

- Heikki