MIME-Version: 1.0
References: 
 <CAO6_XqoxJXddcT4wkd9Xd+cD6Sz-fyspRGuV4Bq-wbXG4pVNzA@mail.gmail.com>
 <0cbf5d34-f117-456f-bcc0-50fa9a8eafba@gmail.com>
 <CAO6_Xqqd316HhigkBsE-rC8oVsAprc_mVh5Z312D+aUOzW-5Jg@mail.gmail.com>
 <MEAPR01MB3031690E672656BB573711B3B665A@MEAPR01MB3031.ausprd01.prod.outlook.com>
 <CAO6_Xqqv5F2cdgs5zCVzLJMOuO6zStsC97gcm41dFNSWOh6B8A@mail.gmail.com>
 <a121b735-70f5-49d9-8774-22a465008545@gmail.com>
 <CAO6_XqrUzBT70kE20uHH=FWyfGJaTDMwqv++yNppdusywsG8-w@mail.gmail.com>
 <bbe64b8a-e61a-4779-a242-eae3e6d2d1f2@gmail.com>
 <CAO6_XqptRqW5b3=BXZ3=OP2YL+7X_CRayCiRwpOySwuHJ2WYtw@mail.gmail.com>
 <1D505B52-9D58-4783-846D-600391C2A3B1@gmail.com>
 <CAO6_Xqqh-tWFMVAuXPnfB8HCGkeBoHK_AgSMZ-qoymb_1NUQjQ@mail.gmail.com>
In-Reply-To: 
 <CAO6_Xqqh-tWFMVAuXPnfB8HCGkeBoHK_AgSMZ-qoymb_1NUQjQ@mail.gmail.com>
From: Fujii Masao <masao.fujii@gmail.com>
Date: Tue, 17 Mar 2026 18:48:46 +0900
Message-ID: 
 <CAHGQGwFSLQZQ+GVnpNDje48D2zpEQQ3nxodHw7Obt0-PmzQNFg@mail.gmail.com>
Subject: Re: Propagate XLogFindNextRecord error to callers
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Cc: Chao Li <li.evan.chao@gmail.com>,
 Mircea Cadariu <cadariu.mircea@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Japin Li <japinli@hotmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAHGQGwFSLQZQ%2BGVnpNDje48D2zpEQQ3nxodHw7Obt0-PmzQNFg%40mail.gmail.com>
Precedence: bulk

On Thu, Feb 26, 2026 at 5:19=E2=80=AFPM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
>
> Thanks for the comments!
>
> On Tue, Feb 24, 2026 at 5:00=E2=80=AFAM Chao Li <li.evan.chao@gmail.com> =
wrote:
> > From a design perspective, I=E2=80=99m not sure we need to add a new er=
rormsg parameter to XLogFindNextRecord(). The new parameter ultimately just=
 exposes state->errormsg_buf, so the returned errormsg implicitly depends o=
n the lifetime of state, and we also need extra handling for cases like err=
ormsg =3D=3D NULL.
> >
> > Instead, perhaps we could add a helper function, say XLogReaderGetLastE=
rror(XLogReaderState *state). which internally pstrdup()s state->errormsg_b=
uf (after checking errormsg_deferred, etc.). That way the caller owns the r=
eturned string explicitly, and there=E2=80=99s no hidden dependency on the =
reader state=E2=80=99s 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 an=
d
            # overwriting its magic number with 0000.
            my $broken_wal_dir =3D 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,

--=20
Fujii Masao