Hi ChangAo, Thanks for the explanation and v2. I re-tested the new version and also ran a comparison against unpatched master to better understand the behavior change.
## Test environment Commit: 64b2b421248 (today's master) OS: Ubuntu 24.04 LTS on AWS EC2 (t3.xlarge, 4 vCPU, 16 GB) Build: --enable-cassert --enable-debug --enable-tap-tests CFLAGS="-O0 -ggdb -fno-omit-frame-pointer" ## Test results for v2 make check: All 248 tests passed make -C src/test/recovery check: All 599 tests passed (262s) Nothing new from the build side — clean compile, no new warnings. ## Manual verification A few observations from exercising pg_switch_wal() repeatedly: -- After some activity that lands mid-segment: SELECT pg_switch_wal(); -> 0/03000078 SELECT pg_current_wal_lsn(); -> 0/04000000 -- Immediately calling switch again (now exactly on segment boundary): SELECT pg_switch_wal(); -> 0/04000000 (no-op, same LSN) SELECT pg_switch_wal(); -> 0/04000000 (still no-op) So pg_switch_wal() is idempotent at segment boundaries — which is what one would hope for. ## Comparison with unpatched master I stashed v2, rebuilt, and re-ran the same sequence on current master. The externally observable LSN behavior is identical: repeated pg_switch_wal() calls at a segment boundary all return the same LSN with no further WAL written. This makes sense: when ReserveXLogSwitch() determines there's no space worth switching, XLogInsertRecord() enters the branch with inserted == false, so the EndPos adjustment logic on the `if (inserted)` path isn't actually reached at segment boundaries in current code. ## My take on v2 The new `EndPos % XLOG_BLCKSZ != 0` guard doesn't fix an externally observable bug in current master — ReserveXLogSwitch() already prevents the problematic case from reaching this code path. But the guard makes the invariant local to XLogInsertRecord() rather than depending on the caller's contract, and the added comment referencing XLogBytePosToEndRecPtr() ties the two paths together nicely. I think that's a worthwhile defensive improvement. Going back to the structure of the original code (rather than the v1 helper-based rewrite) also seems reasonable — no function-call overhead, and the arithmetic is clear enough with the new comment. The MAXALIGN() consistency argument with ReserveXLogSwitch() makes sense to me. Tested-by: Jihyun Bahn <[email protected]> Regards, Jihyun Bahn 2026년 4월 21일 (화) 오후 8:36, cca5507 <[email protected]>님이 작성: > Hi, thanks for the test! > > > One question: > > The original code did not apply MAXALIGN() to SizeOfXLogRecord before > > adding it. In practice SizeOfXLogRecord is likely already MAXALIGN'd > > (given typical record header layout), but could you confirm whether > > MAXALIGN() here is a correctness fix, a defensive no-op, or something > > that requires a separate note in the commit message? > > > > Otherwise the change looks good to me, and I think it's a reasonable > > cleanup. > > I apply the MAXALIGN() to keep it consistent with ReserveXLogSwitch(), the > value seems unchanged though. > > Attach v2 which is more efficient. > > -- > Regards, > ChangAo Chen >
