Hi ChangAo,
I tested v1 of this patch.
Environment:
- PostgreSQL master (commit 9b43e6793b0)
- Ubuntu 24.04, gcc 13.3.0, x86_64
- Configured with --enable-cassert --enable-debug --enable-tap-tests
The patch applies cleanly and builds without new warnings.
Testing performed:
- make check: PASS
- make check-world: PASS
- make -C src/test/recovery check: PASS (599 tests)
In particular, t/
043_no_contrecord_switch.pl and t/
039_end_of_wal.pl both pass, which exercise the XLOG_SWITCH boundary handling.
Manual verification:
I ran pg_switch_wal() twice with some activity in between and observed
that the returned LSN and subsequent pg_current_wal_lsn() values are
consistent with segment boundaries and page headers:
SELECT pg_switch_wal(); -- 0/0178FE38
SELECT pg_current_wal_lsn(); -- 0/02000000 (new segment)
-- ... some DDL and INSERT ...
SELECT pg_switch_wal(); -- 0/02026528
SELECT pg_current_wal_lsn(); -- 0/03000060 (new segment + 0x60
-- == SizeOfXLogLongPHD)
The 0x60 offset matches the long page header size, which is what the
original code was computing via the explicit SizeOfXLogLongPHD /
SizeOfXLogShortPHD branches. The refactored version using
XLogBytePosToEndRecPtr(XLogRecPtrToBytePos(StartPos) +
MAXALIGN(SizeOfXLogRecord)) produces the same result while matching
the pattern used elsewhere in XLogInsertRecord().
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.
Tested-by: Jihyun Bahn <
rring0727@gmail.com>
Regards,
Jihyun Bahn
