>Original >From: Henson Choi <[email protected]> >Date: 2026-06-27 08:23 >To: ZizhuanLiu X-MAN <[email protected]> >Cc: rring0727 <[email protected]>, cca5507 <[email protected]>, pgsql-hackers ><[email protected]> >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)
[email protected]
v5-0001-Keep-the-return-value-of-XLogInsertRecord-for-XLO.patch
Description: Binary data
test-result-v5.txt
Description: Binary data
