Hi Yuriy,
I reviewed and tested v1. The fix looks right to me, and I think it
should go in. Some comments below.
Testing (macOS arm64, Apple clang 17, meson debug build with
assertions, master at f25c50fd8f plus v1 as cfbot applies it):
- The regression tests and the pg_walinspect tests pass.
- Crash recovery with wal_compression = pglz, lz4, zstd and off, each
replaying about 1,100 full-page images with holes, plus a pglz run
with wal_consistency_checking = all (about 21,000 images). The data
matched before and after the crash, and amcheck (verify_heapam, and
bt_index_check with heapallindexed) found nothing.
- I crafted records whose hole doesn't fit in the page (hole_offset
moved so that hole_offset + hole_length = 9192, record CRC
recomputed), once in an uncompressed FPI and once in a
pglz-compressed one. Without the patch, pg_waldump decodes both
records without complaint, and pg_waldump --save-fullpage crashes on
both (SIGBUS and SIGSEGV). With the patch, both are rejected at
decode time, and crash recovery over the same WAL stops there with
BKPIMAGE_HAS_HOLE set, but hole offset 2364 length 6828 block image
length 416 at ...
instead of writing past the page.
Comments:
1. Uncompressed images are affected too, not only compressed ones.
There, hole_length is derived as BLCKSZ - bimg_len, and nothing
checked hole_offset against bimg_len. The new condition covers that
case as well (for an uncompressed image it amounts to requiring
hole_offset <= bimg_len), so the commit message could say so; right
now it only mentions compressed images.
2. v1 doesn't apply to master with git am. The context line in
DecodeXLogRecord() still reads "hole offset %u ... at %X/%X", so I
guess it was made against an older branch. cfbot's copy applies,
but a rebased v2 would make it easier to test.
3. The new message in RestoreBlockImage() uses %X/%X, while everything
else in xlogreader.c uses %X/%08X now. Its text is also identical
to the existing message for a block without an image, so the two
failures can't be told apart in the log.
4. DecodeXLogRecord() is the only place that fills in the image fields
of DecodedBkpBlock, so with this patch the re-check in
RestoreBlockImage() can't fail; in my tests it never did. I'd make
it an Assert(). If you prefer a runtime check, a distinct message
would help (see 3).
About back-patching: XLogRecordAssemble() only creates a hole when
pd_lower >= SizeOfPageHeaderData, pd_upper > pd_lower and
pd_upper <= BLCKSZ, so hole_offset + hole_length <= BLCKSZ for any WAL
that PostgreSQL writes. The new check can't reject valid WAL, and it
turns memory corruption on a bad record into a clean error. So +1 for
back-patching from me.
Unrelated to this patch, but noticed while testing: the error messages
in DecodeXLogRecord() print state->ReadRecPtr rather than the lsn of
the record being decoded. With read-ahead that's an earlier record:
pg_waldump reported the corrupt record at 0/0197B3A0 (the previous
record) instead of 0/0197B7E8, and in recovery the reported LSN was two
records back. I can send a separate patch for that.
It would still be good to see the frontend test you mentioned. I can
also share the script I used to craft the records if that helps.
Regards,
Rahul Yadav
