MIME-Version: 1.0
References: 
 <CACSdjfMQYL3DV-3inrxdReqpFMOky4JxLbN0gTTY+qGJqrUJXw@mail.gmail.com>
 <TYRPR01MB14195CA431CFC783F77FB8AEF94202@TYRPR01MB14195.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TYRPR01MB14195CA431CFC783F77FB8AEF94202@TYRPR01MB14195.jpnprd01.prod.outlook.com>
From: JoongHyuk Shin <sjh910805@gmail.com>
Date: Fri, 17 Apr 2026 16:46:31 +0900
Message-ID: 
 <CACSdjfPtvQ9XQ5QLENHsofbPazqTT55Xufrq7XL_8QNEWtDJ1A@mail.gmail.com>
Subject: Re: [PATCH] Fix TOCTOU race in ReplicationSlotsComputeRequiredLSN()
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000007caa89064fa3269c"
Archived-At: 
 <https://www.postgresql.org/message-id/CACSdjfPtvQ9XQ5QLENHsofbPazqTT55Xufrq7XL_8QNEWtDJ1A%40mail.gmail.com>
Precedence: bulk

--0000000000007caa89064fa3269c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Zhijie,

Thanks for pointing this out.
You're right that my patch only covers the read side
and misses the write side in ReplicationSlotReserveWal().
I should have checked for existing work before submitting.

I'll withdraw this patch.

Best Regards,
JoongHyuk Shin

On Fri, Apr 17, 2026 at 3:59=E2=80=AFPM Zhijie Hou (Fujitsu) <houzj.fnst@fu=
jitsu.com>
wrote:

> On Friday, April 17, 2026 2:50 PM JoongHyuk Shin <sjh910805@gmail.com>
> wrote:
> > Commit 2a5225b99d7 fixed a race in ReplicationSlotsComputeRequiredXmin(=
)
> > where ReplicationSlotControlLock was released before the global xmin
> > update, allowing a concurrent backend to overwrite a correct value with
> > a stale one.
> >
> > ReplicationSlotsComputeRequiredLSN() has the same problem,
> > it releases the lock before calling XLogSetReplicationSlotMinimumLSN(),
> > so a stale minimum LSN can overwrite a correct (lower) one,
> > potentially leading to premature WAL removal.
> >
> > The attached patch moves LWLockRelease() to after the LSN update,
> > matching the xmin fix.
> > Since 2a5225b99d7 was backpatched to all supported versions,
> > I believe this should be as well.
>
> Thanks for noticing this. There is an existing thread [1] that I started
> following 2a5225b99d7 to address the same issue. The patch you posted
> only increases the lock scope in ReplicationSlotsComputeRequiredLSN() but
> does
> not increase the lock level when reserving WALs, so I think it would not
> fix the issue.
>
> Please feel free to review the patch in that thread if you find it helpfu=
l.
>
> [1] https://commitfest.postgresql.org/patch/6451/
>
> Best Regards,
> Hou zj
>

--0000000000007caa89064fa3269c
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Zhijie,<br><br>Thanks for pointing this out.<br>You&#39=
;re right that my patch only covers the read side<br>and misses the write s=
ide in ReplicationSlotReserveWal().<br>I should have checked for existing w=
ork before submitting.<br><br>I&#39;ll withdraw this patch.<br><br>Best Reg=
ards,<br>JoongHyuk Shin</div><br><div class=3D"gmail_quote gmail_quote_cont=
ainer"><div dir=3D"ltr" class=3D"gmail_attr">On Fri, Apr 17, 2026 at 3:59=
=E2=80=AFPM Zhijie Hou (Fujitsu) &lt;<a href=3D"mailto:houzj.fnst@fujitsu.c=
om">houzj.fnst@fujitsu.com</a>&gt; wrote:<br></div><blockquote class=3D"gma=
il_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,2=
04,204);padding-left:1ex">On Friday, April 17, 2026 2:50 PM JoongHyuk Shin =
&lt;<a href=3D"mailto:sjh910805@gmail.com" target=3D"_blank">sjh910805@gmai=
l.com</a>&gt;=C2=A0 wrote:<br>
&gt; Commit 2a5225b99d7 fixed a race in ReplicationSlotsComputeRequiredXmin=
()<br>
&gt; where ReplicationSlotControlLock was released before the global xmin<b=
r>
&gt; update, allowing a concurrent backend to overwrite a correct value wit=
h<br>
&gt; a stale one.<br>
&gt; <br>
&gt; ReplicationSlotsComputeRequiredLSN() has the same problem, <br>
&gt; it releases the lock before calling XLogSetReplicationSlotMinimumLSN()=
, <br>
&gt; so a stale minimum LSN can overwrite a correct (lower) one, <br>
&gt; potentially leading to premature WAL removal.<br>
&gt; <br>
&gt; The attached patch moves LWLockRelease() to after the LSN update,<br>
&gt; matching the xmin fix.<br>
&gt; Since 2a5225b99d7 was backpatched to all supported versions, <br>
&gt; I believe this should be as well.<br>
<br>
Thanks for noticing this. There is an existing thread [1] that I started<br=
>
following 2a5225b99d7 to address the same issue. The patch you posted<br>
only increases the lock scope in ReplicationSlotsComputeRequiredLSN() but d=
oes<br>
not increase the lock level when reserving WALs, so I think it would not<br=
>
fix the issue.<br>
<br>
Please feel free to review the patch in that thread if you find it helpful.=
<br>
<br>
[1] <a href=3D"https://commitfest.postgresql.org/patch/6451/" rel=3D"norefe=
rrer" target=3D"_blank">https://commitfest.postgresql.org/patch/6451/</a><b=
r>
<br>
Best Regards,<br>
Hou zj<br>
</blockquote></div>

--0000000000007caa89064fa3269c--