public inbox for [email protected]  

help / color / mirror / Atom feed

From: Fujii Masao <[email protected]>
To: Anthonin Bonnefoy <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Mircea Cadariu <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Japin Li <[email protected]>
Subject: Re: Propagate XLogFindNextRecord error to callers
Date: Tue, 17 Mar 2026 18:48:46 +0900
Message-ID: <CAHGQGwFSLQZQ+GVnpNDje48D2zpEQQ3nxodHw7Obt0-PmzQNFg@mail.gmail.com> (raw)
In-Reply-To: <CAO6_Xqqh-tWFMVAuXPnfB8HCGkeBoHK_AgSMZ-qoymb_1NUQjQ@mail.gmail.com>
References: <CAO6_XqoxJXddcT4wkd9Xd+cD6Sz-fyspRGuV4Bq-wbXG4pVNzA@mail.gmail.com>
	<[email protected]>
	<CAO6_Xqqd316HhigkBsE-rC8oVsAprc_mVh5Z312D+aUOzW-5Jg@mail.gmail.com>
	<MEAPR01MB3031690E672656BB573711B3B665A@MEAPR01MB3031.ausprd01.prod.outlook.com>
	<CAO6_Xqqv5F2cdgs5zCVzLJMOuO6zStsC97gcm41dFNSWOh6B8A@mail.gmail.com>
	<[email protected]>
	<CAO6_XqrUzBT70kE20uHH=FWyfGJaTDMwqv++yNppdusywsG8-w@mail.gmail.com>
	<[email protected]>
	<CAO6_XqptRqW5b3=BXZ3=OP2YL+7X_CRayCiRwpOySwuHJ2WYtw@mail.gmail.com>
	<[email protected]>
	<CAO6_Xqqh-tWFMVAuXPnfB8HCGkeBoHK_AgSMZ-qoymb_1NUQjQ@mail.gmail.com>

On Thu, Feb 26, 2026 at 5:19 PM Anthonin Bonnefoy
<[email protected]> wrote:
>
> Thanks for the comments!
>
> On Tue, Feb 24, 2026 at 5:00 AM Chao Li <[email protected]> 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

view thread (13+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Propagate XLogFindNextRecord error to callers
  In-Reply-To: <CAHGQGwFSLQZQ+GVnpNDje48D2zpEQQ3nxodHw7Obt0-PmzQNFg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox