Re: POC: make mxidoff 64 bits - Mailing list pgsql-hackers

On Mon, Nov 17, 2025 at 10:05 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> Ashutosh, you were interested in reviewing this earlier. Would you have
> a chance to review this now, before I commit it?

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.

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. For example, it's not
clear which offset MultiXactIdToOffset* functions are about. These
functions are calculating the page, entry (within the page) and
segment (of page) in pg_multixact/offset where to find the
MultiXactOffset of the first member of a given mxid. Thus returning
offset within offset. I feel they should have been named differently
when the code was written. But now that we are moving this code in a
separate file, we also have an opportunity to rename it better. I
think MXOffsetToMember* functions have better names. Using a similar
convention we could use MultiXactIdToOffsetOffset*, but that might
increase confusion. How about MultiXactIdToOffsetPos*? A separate .h
file also looks like a good place to document how offsets are laid out
and its contents and how members is laid out. The latter is somehow
documented in terms of macros and the static functions. The first is
not documented well, I feel. This refactoring seems to be a good
opportunity to do that. If we do so, I think, the .h there will be
some value in committing .h file as a separate commit.

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.

ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("MultiXact members would wrap around")));

 If a server ever reaches this point, there is no way but to create
another cluster, if the applications requires multi-xact ids? We
should also provide this as an errhint().

if (nextOffset + nmembers < nextOffset)

:). I spent a few seconds trying to understand this. But I don't know
how to make it easy to understand.

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?

PerformMembersTruncation(MultiXactOffset oldestOffset, MultiXactOffset
newOldestOffset)
{
... snip ...
- segment += 1;
- }
+ SimpleLruTruncate(MultiXactMemberCtl,
+ MXOffsetToMemberPage(newOldestOffset));
}

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.

MultiXactState->oldestMultiXactId = newOldestMulti;
MultiXactState->oldestMultiXactDB = newOldestMultiDB;
+ MultiXactState->oldestOffset = newOldestOffset;
LWLockRelease(MultiXactGenLock);

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.

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.

--
Best Wishes,
Ashutosh Bapat