From: =?utf-8?B?0J3QuNC60LjRgtCwINCa0LDQu9C40L3QuNC9?=
 <n.kalinin@postgrespro.ru>
Message-Id: <98783F14-E583-4CFB-9AE0-7B3945449724@postgrespro.ru>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_D2F57EA2-76A7-40E7-97C3-08C2CDB3BCFD"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.600.51.1.1\))
Subject: Re: BUG #19500: pgrepack logical decoding plugin can crash assert
 builds via SQL decoding API
Date: Tue, 2 Jun 2026 09:39:26 +0700
In-Reply-To: <ah2296X21zeqsylc@alvherre.pgsql>
Cc: pgsql-bugs@lists.postgresql.org,
 Antonin Houska <ah@cybertec.at>,
 b@ida.kurilemu.internal
To: =?utf-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>
References: <ah2296X21zeqsylc@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/98783F14-E583-4CFB-9AE0-7B3945449724%40postgrespro.ru>
Precedence: bulk


--Apple-Mail=_D2F57EA2-76A7-40E7-97C3-08C2CDB3BCFD
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

> On 2 Jun 2026, at 00:12, =C3=81lvaro Herrera <alvherre@kurilemu.de> =
wrote:
>=20
> How about something like this?  It makes your test case throw an error
> instead of failing the assertion, which I suppose is an improvement.
>=20
> The patch is a bit noisy because I moved more code than the minimum
> necessary; but the gist of it is that we allocate RepackDecodingState =
in
> repack_startup(), then have repack_setup_logical_decoding() fill in a
> magic number, which we later check in repack_begin_txn().  This is a =
bit
> wasteful, because we have to do that check once for each and every
> transaction; however I see no other callback that would let us do this
> kind of check after the slot is created but before we start to consume
> from it.
>=20
> --=20
> =C3=81lvaro Herrera               48=C2=B001'N 7=C2=B057'E  =E2=80=94  =
https://www.EnterpriseDB.com/
> "Before you were born your parents weren't as boring as they are now. =
They
> got that way paying your bills, cleaning up your room and listening to =
you
> tell them how idealistic you are."  -- Charles J. Sykes' advice to =
teenagers
> <0001-Have-RepackDecodingState-carry-a-magic-number.patch>

Yes, I agree that returning an error to the user makes sense.=20

But does the error message need to be that detailed? Perhaps something =
like

"ERROR: wrong magic number in "pgrepack" decoder plugin"
would be sufficient.

Nevertheless, I tested the patch and can confirm that there are no =
assertion failures anymore.=20

I also ran it under ASAN and did not observe any issues.

Would it make sense to add a test for this case from the bug report?


--Apple-Mail=_D2F57EA2-76A7-40E7-97C3-08C2CDB3BCFD
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html aria-label=3D"message body"><head><meta http-equiv=3D"content-type" =
content=3D"text/html; charset=3Dutf-8"></head><body =
style=3D"overflow-wrap: break-word; -webkit-nbsp-mode: space; =
line-break: after-white-space;"><div><blockquote type=3D"cite"><div>On 2 =
Jun 2026, at 00:12, =C3=81lvaro Herrera &lt;alvherre@kurilemu.de&gt; =
wrote:</div><div><div><br>How about something like this? &nbsp;It makes =
your test case throw an error<br>instead of failing the assertion, which =
I suppose is an improvement.<br><br>The patch is a bit noisy because I =
moved more code than the minimum<br>necessary; but the gist of it is =
that we allocate RepackDecodingState in<br>repack_startup(), then have =
repack_setup_logical_decoding() fill in a<br>magic number, which we =
later check in repack_begin_txn(). &nbsp;This is a bit<br>wasteful, =
because we have to do that check once for each and every<br>transaction; =
however I see no other callback that would let us do this<br>kind of =
check after the slot is created but before we start to consume<br>from =
it.<br><br>-- <br>=C3=81lvaro Herrera =
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&n=
bsp;&nbsp;48=C2=B001'N 7=C2=B057'E &nbsp;=E2=80=94 =
&nbsp;https://www.EnterpriseDB.com/<br>"Before you were born your =
parents weren't as boring as they are now. They<br>got that way paying =
your bills, cleaning up your room and listening to you<br>tell them how =
idealistic you are." &nbsp;-- Charles J. Sykes' advice to =
teenagers<br><span =
id=3D"cid:C8E60D49-67A4-43E3-BA6A-665134F06A64">&lt;0001-Have-RepackDecodi=
ngState-carry-a-magic-number.patch&gt;</span></div></div></blockquote></di=
v><br><div><p data-start=3D"14" data-end=3D"151">Yes, I agree that =
returning an error to the user makes sense.&nbsp;</p><p data-start=3D"14" =
data-end=3D"151">But does the error message need to be that detailed? =
Perhaps something like</p><pre class=3D"overflow-visible! px-0!" =
data-start=3D"156" data-end=3D"226"><div class=3D"relative w-full mt-4 =
mb-1"><div><div class=3D"relative"><div class=3D"h-full min-h-0 =
min-w-0"><div class=3D"h-full min-h-0 min-w-0"><div class=3D"border =
border-token-border-light border-radius-3xl corner-superellipse/1.1 =
rounded-3xl"><div class=3D"h-full w-full border-radius-3xl =
bg-token-bg-elevated-secondary corner-superellipse/1.1 overflow-clip =
rounded-3xl lxnfua_clipPathFallback"><div class=3D"pointer-events-none =
absolute end-1.5 top-1 z-2 md:end-2 md:top-1"></div><div =
class=3D"relative"><div class=3D"pe-11 pt-3"><div class=3D"relative z-0 =
flex max-w-full"><div id=3D"code-block-viewer" dir=3D"ltr" =
class=3D"q9tKkq_viewer cm-editor z-10 light:cm-light dark:cm-light flex =
h-full w-full flex-col items-stretch =CD=BCd =CD=BCr"><div =
class=3D"cm-scroller"><pre class=3D"cm-content q9tKkq_readonly =
m-0"><code>"ERROR: wrong magic number in "pgrepack" decoder =
plugin"</code></pre></div></div></div></div></div></div></div></div></div>=
<div><div></div></div></div></div></div></pre><p data-start=3D"231" =
data-end=3D"251">would be sufficient.</p><p data-start=3D"256" =
data-end=3D"407">Nevertheless, I tested the patch and can confirm that =
there are no assertion failures anymore.&nbsp;</p><p data-start=3D"256" =
data-end=3D"407">I also ran it under ASAN and did not observe any =
issues.</p><p data-start=3D"412" data-end=3D"469">Would it make sense to =
add a test for this case from the bug report?</p><p data-start=3D"412" =
data-end=3D"469"><br></p></div></body></html>=

--Apple-Mail=_D2F57EA2-76A7-40E7-97C3-08C2CDB3BCFD--