Subject: Propagate XLogFindNextRecord error to callers

From: Anthonin Bonnefoy <[email-masked]>
Date: Fri, 12 Dec 2025 11:39:50 +0100

Hi,

Currently, XLogFindNextRecord errormsg is ignored and callers will only
output a generic 'could not find a valid record' message without details.
Additionally, invalid page header won't go through XLogReadRecord, leaving
the error in state->errormsg_buf.

This patch propagates XLogFindNextRecord's error message to the callers and
displays it. In case of an invalid page header, the errormsg is filled with
errormsg_buf content.

With this patch, pg_waldump will now have the following output when reading
a file with an invalid header:
pg_waldump: error: could not find a valid record after D80/5C000000:
invalid magic number D116 in WAL segment 0000001400000D8000000017, LSN
D80/5C000000, offset 0

Regards,
Anthonin Bonnefoy

---

From: Fujii Masao <[email-masked]>
Date: Tue, 17 Mar 2026 18:48:46 +0900

On Thu, Feb 26, 2026 at 5:19 PM Anthonin Bonnefoy
<[email-masked]> wrote:
>
> Thanks for the comments!
>
> On Tue, Feb 24, 2026 at 5:00 AM Chao Li <[email-masked]> wrote:
> > From a design perspective, I’m not sure we need to add a new errormsg parameter to XLogFindNextRecord(). The new parameter ultimately just exposes state->errormsg_buf, so the returned errormsg implicitly depends on the lifetime of state, and we also need extra handling for cases like errormsg == NULL.
> >
> > Instead, perhaps we could add a helper function, say XLogReaderGetLastError(XLogReaderState *state). which internally pstrdup()s state->errormsg_buf (after checking errormsg_deferred, etc.). That way the caller owns the returned string explicitly, and there’s no hidden dependency on the reader state’s lifetime.
> >
> > This would also avoid changing the XLogFindNextRecord() signature while making the ownership semantics clearer.
>
> One issue I see is that it introduces another way to get the error,
> with XLogReadRecord and XLogNextRecord using an errormsg parameter,
> and XLogFindNextRecord using the helper function. Maybe the solution
> would be to change both XLogReadRecord and XLogNextRecord to use this
> new function to stay consistent, but that means changing their
> signatures.
>
> Also, I see the errormsg parameter as a way to signal the caller that
> "this function can fail, the detailed error will be available here".
> With the XLogReaderGetLastError, it becomes the caller's
> responsibility to know which function may fill the error message and
> check it accordingly.
>
> The error message is likely printed shortly after the function's call,
> so I suspect the risk of using the errormsg after its intended
> lifetime is low.
>
> You bring up a good point about the errormsg's lifetime, which is
> definitely something to mention in the function's comments. I've
> updated the patch with the additional comments.

Since this patch is marked Ready for Committer, I've started reviewing it.

+ * 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()?

+# 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();

+ 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: $!");

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

Regards,

-- 
Fujii Masao

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Mon, 23 Mar 2026 09:15:13 +0100

> Since this patch is marked Ready for Committer, I've started reviewing it.

Thanks for the review!

> "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.

>             my $broken_wal_dir = PostgreSQL::Test::Utils::tempdir_short();

Sounds good, I've updated the patch.

>         or BAIL_OUT("close failed: $!");

Added.

> Also, other similar tests seem to call binmode. Is it really unnecessary

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

---

From: Fujii Masao <[email-masked]>
Date: Mon, 23 Mar 2026 21:00:00 +0900

> Sounds good, I've updated the patch.

Thanks for updating the patch!

It looks like the first part of the comment above wasn't included,
so I've added it. I also made a few cosmetic changes and updated
the commit message. Attached is the updated version of the patch.

Unless there are objections, I'll commit this version.

Regards,

-- 
Fujii Masao

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Mon, 23 Mar 2026 13:07:39 +0100

> the commit message. Attached is the updated version of the patch.

Right, I've missed the first part... Thanks for the fix!

---

From: Fujii Masao <[email-masked]>
Date: Tue, 24 Mar 2026 22:35:15 +0900

> Right, I've missed the first part... Thanks for the fix!

I've pushed the patch. Thanks!

Regards,

-- 
Fujii Masao