Re: Propagate XLogFindNextRecord error to callers

On Tue, Mar 17, 2026 at 10:49 AM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Since this patch is marked Ready for Committer, I've started reviewing it.

Thanks for the review!

On Tue, Mar 17, 2026 at 10:49 AM Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> + * When set, *errormsg points to an internal buffer that's valid until the next
> + * call to XLogReadRecord.
>
> Could that buffer also be invalidated by other functions that modify
> "XLogReaderState *state", such as XLogBeginRead()?

Yes, XLogBeginRead() will clear the buffer, but I don't think there's
a pattern where XLogBeginRead() is called after XLogFindNextRecord()
on the same reader?
The comment was more focused on how the function would be used, with
XLogReadRecord() being called after the reader state is initialized,
and talking about XLogBeginRead() here could be confusing.

> +# Wrong WAL version. We copy an existing wal file and set the
> +# page's magic value to 0000.
>
> Would it be better to describe the purpose of this test at the top?
> For example:
>
> # Test that pg_waldump reports a detailed error message when dumping
> # a WAL file with an invalid magic number (0000).
> {
> # The broken WAL file is created by copying a valid WAL file and
> # overwriting its magic number with 0000.
> my $broken_wal_dir = PostgreSQL::Test::Utils::tempdir_short();

Sounds good, I've updated the patch.

> + open($fh, '+<', $broken_wal);
> + close($fh);
>
> Should we add error handling like:
>
> open(my $fh, '+<', $broken_wal)
> or BAIL_OUT("open($broken_wal) failed: $!");
> close($fh)
> or BAIL_OUT("close failed: $!");

Added.

> Also, other similar tests seem to call binmode. Is it really unnecessary
> in this case?

Ha right, I've missed those calls. I'm not sure if it's strictly
necessary for the functions used there, but it's probably a lot saner
to read and write WAL using binary mode. I've added a binmode call.

Regards,
Anthonin Bonnefoy