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 <
rring0727@gmail.com>
Regards,
Jihyun Bahn
