Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\))
Subject: Re: Propagate XLogFindNextRecord error to callers
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <CAO6_XqptRqW5b3=BXZ3=OP2YL+7X_CRayCiRwpOySwuHJ2WYtw@mail.gmail.com>
Date: Tue, 24 Feb 2026 12:00:05 +0800
Cc: Mircea Cadariu <cadariu.mircea@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Japin Li <japinli@hotmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <1D505B52-9D58-4783-846D-600391C2A3B1@gmail.com>
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>
To: Anthonin Bonnefoy <anthonin.bonnefoy@datadoghq.com>
Archived-At: <https://www.postgresql.org/message-id/1D505B52-9D58-4783-846D-600391C2A3B1%40gmail.com>
Precedence: bulk


> On Feb 12, 2026, at 15:43, Anthonin Bonnefoy =
<anthonin.bonnefoy@datadoghq.com> wrote:
>=20
> On Wed, Feb 11, 2026 at 1:16=E2=80=AFPM Mircea Cadariu =
<cadariu.mircea@gmail.com> wrote:
>> Indeed. Should we place this initialisation at the top of
>> XLogFindNextRecord, before any processing? At that point, there's
>> nothing to erase.
>=20
> That makes sense, I've added the '*errormsg =3D NULL' at the top of =
the function.
>=20
>> You can consider capitalising.
>=20
> Done
> <v5-0001-Propagate-errormsg-to-XLogFindNextRecord-caller.patch>

Hi Anthonin,

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

=46rom a design perspective, I=E2=80=99m 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 =3D=3D 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=E2=80=99=
s 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.

What do you think?

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