Subject: Re: Propagate XLogFindNextRecord error to callers

From: Mircea Cadariu <[email-masked]>
Date: Mon, 9 Feb 2026 14:15:34 +0200

Hi Anthonin,

Thanks for the updated patch.

I have noticed this code added in XLogFindNextRecord in the patch, 
appears also in XLogNextRecord (line 334).

> +	if (state->errormsg_deferred)
> +	{
> +		if (state->errormsg_buf[0] != '\0')
> +			*errormsg = state->errormsg_buf;
> +		state->errormsg_deferred = false;
> +	}
> +

In XLogNextRecord, right before the above code, we do *errormsg = NULL. 
Should this be done also in XLogFindNextRecord in the patch?

If so, what about even extracting a helper method which will be called 
from both places?

A nit for the commit message: Propage -> Propagate

-- 
Thanks,
Mircea Cadariu

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Wed, 11 Feb 2026 09:43:51 +0100

On Mon, Feb 9, 2026 at 1:15 PM Mircea Cadariu <[email-masked]> wrote:
> In XLogNextRecord, right before the above code, we do *errormsg = NULL. Should this be done also in XLogFindNextRecord in the patch?
>
> If so, what about even extracting a helper method which will be called from both places?

XLogReadRecord may already have consumed errormsg_deferred and set
errormsg. We can't set it to NULL, or that would erase a valid error
message.

A simplification would have been to remove processing the deferred
error processing from XLogNextRecord and leave it to
XLogFindNextRecord, but there are some calls to XLogNextRecord (like
in xlogprefetcher.c), so being able to get the error message when
calling XLogNextRecord is necessary.

> A nit for the commit message: Propage -> Propagate

Fixed

---

From: Mircea Cadariu <[email-masked]>
Date: Wed, 11 Feb 2026 14:15:56 +0200

> errormsg. We can't set it to NULL, or that would erase a valid error
Indeed. Should we place this initialisation at the top of 
XLogFindNextRecord, before any processing? At that point, there's 
nothing to erase.

> +	/*
> +	 * we may have reported errors due to invalid WAL header, propagate the
> +	 * error message to the caller.
> +	 */

You can consider capitalising.

-- 
Thanks,
Mircea Cadariu

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Thu, 12 Feb 2026 08:43:03 +0100

> XLogFindNextRecord, before any processing? At that point, there's

That makes sense, I've added the '*errormsg = NULL' at the top of the function.

> You can consider capitalising.

Done

---

From: Mircea Cadariu <[email-masked]>
Date: Tue, 17 Feb 2026 19:48:18 +0000

Hi,

It applies cleanly on HEAD and the tests pass locally, as well as in CI.

I've set the patch entry to 'Ready for Committer'.

-- 
Thanks,
Mircea Cadariu

---

From: Chao Li <[email-masked]>
Date: Tue, 24 Feb 2026 12:00:05 +0800

> On Feb 12, 2026, at 15:43, Anthonin Bonnefoy <[email-masked]> wrote:
> 
> <v5-0001-Propagate-errormsg-to-XLogFindNextRecord-caller.patch>

Hi Anthonin,

Thanks for the patch. I agree it’s useful to print a more detailed error message instead of the generic one.

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.

What do you think?

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Anthonin Bonnefoy <[email-masked]>
Date: Thu, 26 Feb 2026 09:19:09 +0100

Thanks for the comments!

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

Regards,
Anthonin Bonnefoy

---

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

> 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