Re: Return value of XLogInsertRecord() for XLOG_SWITCH record

>Original
>From: Henson Choi <assam258(at)gmail(dot)com>
>Date: 2026-06-27 08:23
>To: ZizhuanLiu X-MAN <44973863(at)qq(dot)com>
>Cc: rring0727 <rring0727(at)gmail(dot)com>, cca5507 <cca5507(at)qq(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
>Subject: Re: Return value of XLogInsertRecord() for XLOG_SWITCH record
>
>Hi ZizhuanLiu,
>
>> Rebase v4 .
>
>Thanks for the rebase. I read through the v4 refactor of
>ReserveXLogSwitch() -- computing the standard EndPos once and reusing it
>reads much cleaner than the previous duplicated arithmetic. A few small,
>behavior-neutral suggestions on the new out-parameter, in case you'd like
>to fold them into the next version:
>
>1) Naming. Of the three, this is the one I'd really like to see in the
>next version. The parameter is spelled actual_EndPos, which is neither
>snake_case nor CamelCase -- an underscore-separated lowercase prefix glued
>to a CamelCase tail -- unlike its sibling out-parameters in the same
>function (StartPos, EndPos, PrevPtr) that are plain CamelCase; and
>"actual" doesn't say what it is actual relative to.
>Since it carries the end of the record itself -- as opposed to EndPos,
>which is padded out to the segment boundary -- I'd suggest RecEndPos,
>which matches the surrounding names:
>
> static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
> XLogRecPtr *PrevPtr, XLogRecPtr *RecEndPos);
>
>2) Drop the NULL guard on the out-parameter. There is a single caller and
>it always passes &..., exactly like the other three out-parameters, which
>are dereferenced unconditionally. Guarding only this one is inconsistent:
>
> /* Store the end position of just the record. */
> *RecEndPos = *EndPos;
>
>To keep that unconditional store safe, the early "already at a segment
>boundary" return should set it too:
>
> if (XLogSegmentOffset(ptr, wal_segment_size) == 0)
> {
> SpinLockRelease(&Insert->insertpos_lck);
> *EndPos = *StartPos = ptr;
> *RecEndPos = ptr;
> return false;
> }
>
>3) With RecEndPos always populated, the `if (inserted)` guard around the
>EndPos fixup in XLogInsertRecord() is no longer needed -- when nothing was
>inserted RecEndPos already equals EndPos, so the assignment is a harmless
>no-op:
>
> /*
> * Even though we reserved the rest of the segment for us, which is
> * reflected in EndPos, we return a pointer to just the end of the
> * xlog-switch record, which is consistent with other WAL types
> * returned by XLogBytePosToEndRecPtr(). When no switch record was
> * inserted, RecEndPos already equals EndPos, so this is a no-op.
> */
> EndPos = RecEndPos;
>
>None of this changes behavior; it just localizes the invariant inside
>ReserveXLogSwitch() and makes the out-parameters uniform. (2) and (3) are
>take-it-or-leave-it -- the rename is the only one I feel strongly about.
>Feel free to fold in whichever you find worthwhile for the next version.
>
>Regards,
>Henson

Hi, Henson

Thank you for your review and detailed suggestions.

Point 1) is a great catch; this was an oversight on my part.
I really appreciate the elegant code design proposed in points 2) and 3).
Thank you very much for your detailed guidance.

I have unified the assignment logic inside `ReserveXLogSwitch()` with
`*RecEndPos = *EndPos;` to keep the code consistent.

I have compiled and tested v5, and the results are as expected.

I would like to add some detailed clarification regarding the test case
"4. Cross page boundaries within a single WAL segment file". After
setting `Insert->CurrBytePos` via GDB, the correct derivation for the
expected value of `EndPos` is as follows (v4 used 4 bytes, while v5 uses 8 bytes):
```
EndPos == (Current XLOG_BLCK) StartPos + 8 (first 8 bytes of MAXALIGN(SizeOfXLogRecord))
+ (Next XLOG_BLCK) 24 (SizeOfXLogShortPHD) + 16 (the remaining bytes of MAXALIGN(SizeOfXLogRecord))
```

regards,
--
ZizhuanLiu (X-MAN)
44973863(at)qq(dot)com