MIME-Version: 1.0
References: <19490-9c59c6a583513b99@postgresql.org>
 <46FE61C9-F273-45FD-BED7-0F8CDA6EB992@yandex-team.ru>
 <CAL9smLBMxKBCmsA9UGcmf93bT2_MsZ+POH-oHREuwKdmMU7jfQ@mail.gmail.com>
 <46DB3CAB-EA1C-41A5-9D6D-5F913A2AAF66@yandex-team.ru>
 <CAJgoLkJfFgL-V+pYB7=R81AbURTE6sMhzVHDQDhVGnfXRSJ9Wg@mail.gmail.com>
 <CAJgoLkKCu0wCwPQZSo5no=XATU-4LMK4QfKBwV928o2uKcxe=g@mail.gmail.com>
 <CAJTYsWU6tdEvVh4YKLxz7+amZ7+Wb7_s-FBjsMMeLNj1fKeSNg@mail.gmail.com>
 <ahVTk8HKRPcz4iZ-@paquier.xyz>
In-Reply-To: <ahVTk8HKRPcz4iZ-@paquier.xyz>
From: Ayush Tiwari <ayushtiwari.slg01@gmail.com>
Date: Tue, 26 May 2026 14:00:30 +0530
Message-ID: 
 <CAJTYsWWXvbBJe+WYJZcnoSTyVz9vk5ro3x2qAq_uvXvK2KwaMQ@mail.gmail.com>
Subject: Re: BUG #19490: Streaming standby on 16.14 stops applying WAL on
 MultiXactOffsetSLRU when primary is 16.8
To: Michael Paquier <michael@paquier.xyz>
Cc: Radim Marek <radim@boringsql.com>, Andrey Borodin <x4mmm@yandex-team.ru>,
	Heikki Linnakangas <hlinnaka@iki.fi>, Marko Tiikkaja <marko@joh.to>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000084ab730652b44f69"
Archived-At: 
 <https://www.postgresql.org/message-id/CAJTYsWWXvbBJe%2BWYJZcnoSTyVz9vk5ro3x2qAq_uvXvK2KwaMQ%40mail.gmail.com>
Precedence: bulk

--00000000000084ab730652b44f69
Content-Type: text/plain; charset="UTF-8"

Hi,

On Tue, 26 May 2026 at 13:32, Michael Paquier <michael@paquier.xyz> wrote:

> On Fri, May 22, 2026 at 10:21:32PM +0530, Ayush Tiwari wrote:
> > I think the right fix is to remove that SimpleLruWriteAll() call while
> > keeping the missing-page initialization logic.  The flush is only meant
> to
> > make SimpleLruDoesPhysicalPageExist() see pages that exist in SLRU
> buffers
> > but have not reached disk.  In this fallback path, I don't see a way for
> > the tested next_pageno to be in that state: if RecordNewMultiXact()
> itself
> > initializes the page, it writes it synchronously with
> SimpleLruWritePage()
> > before setting last_initialized_offsets_page.
>
> FWIW, I'm having a couple of customers complaining about that as well,
> as cross-version physical replication is a thing for minor upgrade
> flows.  This bug is making suddenly recovery disruptive for some folks
> out there.  :(
>

We had faced a lot of replicas in bad state due to multixact replay with
16.12 release, and had to revert back the minor versions for them  until
16.13 came out which was a blessing. Given the number of CVEs
current one fixes, reverting too is scary.


> > I attached a small patch for REL_16_STABLE.  The same self-deadlock
> pattern
> > is also present on PG 14 and 15.  PG 17 and
> > 18 have the same compatibility call, but SLRU locking is banked
> > there, and RecordNewMultiXact() does not appear to hold the relevant bank
> > lock before calling SimpleLruWriteAll(), so I would not describe those
> > branches as having this exact self-deadlock, but needs more analysis.
>
> So your root argument is that while the SimpleLruWriteAll() is
> defensive, it is not actually necessary because it means that
> last_initialized_offsets_page is -1 we have not yet replayed
> ZERO_OFF_PAGE and that we have no dirty page that could make
> SimpleLruDoesPhysicalPageExis() return an incorrect result, which
> would be bad.  I am not sure to agree that this assumption is correct
> all the time, see for example the WAL message mentioned in the thread
> that has led to 77dff5d937b1:
>
> https://www.postgresql.org/message-id/33319276-e4d0-4773-89e4-09084905fdb0%40iki.fi


Right, agreed.  Thanks for pointing to that case.  My v1 patch removes
the self-deadlock, but the "no relevant dirty pages" assumption is too
strong.

The dirty page does not have to be one initialized by the current
RecordNewMultiXact() call.  It can already contain offsets replayed from
later CREATE_ID records while last_initialized_offsets_page is still -1.
In that state, relying directly on SimpleLruDoesPhysicalPageExist() can
still produce a false negative because it only checks the physical file,
not dirty SLRU buffers.  So removing the flush can maybe reintroduce
the kind of corruption that 77dff5d937b1 was trying to prevent.

A different approach would be to release and re-acquire the
> MultiXactOffsetSLRULock while calling SimpleLruWriteAll(), and I think
> that it should be actually safe.  Even if read-only backends evict
> dirty pages between the moment the lock is released and the moment it
> is re-acquired in SimpleLruWriteAll(), the pages would be would be
> written to disk due to the eviction, which is what we want for
> correctness.  And only the startup process dirties offset pages during
> recovery, AFAIK.  Thoughts?
>

 That sounds like the right direction to me.

Releasing MultiXactOffsetSLRULock around SimpleLruWriteAll() preserves
the flush-before-physical-check rule while avoiding the self-deadlock.
I don't see a partial-state problem from the current record at that
point, since the compatibility check happens before RecordNewMultiXact()
has modified the current offsets page.  And as you said, during recovery
The startup process should be the only process dirtying offset pages; if
a hot standby reader causes eviction while the lock is released, that
should only help by writing the dirty page out.

> Added both Andrey and Heikki in to-mail, since I'm not sure if this
> > is more extreme than the multixact offset issue we had with 16.12, or it
> > is at par with that.
>
> Indeed, let's wait for at least Heikki's input.
>
> Anyway, for any fixes, I don't think that it would be a good idea to
> skip v17 and v18, relying on the SLRU bank locks to not conflict to
> bypass the WriteAll() conflict.  Let's keep all the branches across
> v14~v18 in sync.
>

Agreed.

Regards,
Ayush

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi,</div><br><div class=3D"gmail_quote gm=
ail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Tue, 26 May 2=
026 at 13:32, Michael Paquier &lt;<a href=3D"mailto:michael@paquier.xyz">mi=
chael@paquier.xyz</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote"=
 style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);p=
adding-left:1ex">On Fri, May 22, 2026 at 10:21:32PM +0530, Ayush Tiwari wro=
te:<br>
&gt; I think the right fix is to remove that SimpleLruWriteAll() call while=
<br>
&gt; keeping the missing-page initialization logic.=C2=A0 The flush is only=
 meant to<br>
&gt; make SimpleLruDoesPhysicalPageExist() see pages that exist in SLRU buf=
fers<br>
&gt; but have not reached disk.=C2=A0 In this fallback path, I don&#39;t se=
e a way for<br>
&gt; the tested next_pageno to be in that state: if RecordNewMultiXact() it=
self<br>
&gt; initializes the page, it writes it synchronously with SimpleLruWritePa=
ge()<br>
&gt; before setting last_initialized_offsets_page.<br>
<br>
FWIW, I&#39;m having a couple of customers complaining about that as well,<=
br>
as cross-version physical replication is a thing for minor upgrade<br>
flows.=C2=A0 This bug is making suddenly recovery disruptive for some folks=
<br>
out there.=C2=A0 :(<br></blockquote><div><br></div><div>We had faced a lot =
of replicas in bad state due to multixact replay with</div><div>16.12 relea=
se, and had to revert back the minor versions for them=C2=A0 until=C2=A0</d=
iv><div>16.13 came out which was a blessing. Given the number of CVEs=C2=A0=
</div><div>current one fixes, reverting too is scary.</div><div>=C2=A0</div=
><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border=
-left:1px solid rgb(204,204,204);padding-left:1ex">
&gt; I attached a small patch for REL_16_STABLE.=C2=A0 The same self-deadlo=
ck pattern<br>
&gt; is also present on PG 14 and 15.=C2=A0 PG 17 and<br>
&gt; 18 have the same compatibility call, but SLRU locking is banked<br>
&gt; there, and RecordNewMultiXact() does not appear to hold the relevant b=
ank<br>
&gt; lock before calling SimpleLruWriteAll(), so I would not describe those=
<br>
&gt; branches as having this exact self-deadlock, but needs more analysis.<=
br>
<br>
So your root argument is that while the SimpleLruWriteAll() is<br>
defensive, it is not actually necessary because it means that<br>
last_initialized_offsets_page is -1 we have not yet replayed<br>
ZERO_OFF_PAGE and that we have no dirty page that could make<br>
SimpleLruDoesPhysicalPageExis() return an incorrect result, which<br>
would be bad.=C2=A0 I am not sure to agree that this assumption is correct<=
br>
all the time, see for example the WAL message mentioned in the thread<br>
that has led to 77dff5d937b1:<br>
<a href=3D"https://www.postgresql.org/message-id/33319276-e4d0-4773-89e4-09=
084905fdb0%40iki.fi" rel=3D"noreferrer" target=3D"_blank">https://www.postg=
resql.org/message-id/33319276-e4d0-4773-89e4-09084905fdb0%40iki.fi</a></blo=
ckquote><div><br></div>Right, agreed.=C2=A0 Thanks for pointing to that cas=
e.=C2=A0 My v1 patch removes<br>the self-deadlock, but the &quot;no relevan=
t dirty pages&quot; assumption is too<br>strong.<br><br>The dirty page does=
 not have to be one initialized by the current<br>RecordNewMultiXact() call=
.=C2=A0 It can already contain offsets replayed from<br>later CREATE_ID rec=
ords while last_initialized_offsets_page is still -1.<br>In that state, rel=
ying directly on SimpleLruDoesPhysicalPageExist() can<br>still produce a fa=
lse negative because it only checks the physical file,<br>not dirty SLRU bu=
ffers.=C2=A0 So removing the flush can maybe reintroduce=C2=A0</div><div cl=
ass=3D"gmail_quote gmail_quote_container">the kind of corruption that 77dff=
5d937b1 was trying to prevent.<div><br></div><blockquote class=3D"gmail_quo=
te" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204=
);padding-left:1ex">
A different approach would be to release and re-acquire the<br>
MultiXactOffsetSLRULock while calling SimpleLruWriteAll(), and I think<br>
that it should be actually safe.=C2=A0 Even if read-only backends evict<br>
dirty pages between the moment the lock is released and the moment it<br>
is re-acquired in SimpleLruWriteAll(), the pages would be would be<br>
written to disk due to the eviction, which is what we want for<br>
correctness.=C2=A0 And only the startup process dirties offset pages during=
<br>
recovery, AFAIK.=C2=A0 Thoughts?<br></blockquote><div><br></div><div>=C2=A0=
That sounds like the right direction to me.<br><br></div><div>Releasing Mul=
tiXactOffsetSLRULock around SimpleLruWriteAll() preserves<br>the flush-befo=
re-physical-check rule while avoiding the self-deadlock.<br>I don&#39;t see=
 a partial-state problem from the current record at that<br>point, since th=
e compatibility check happens before RecordNewMultiXact()<br>has modified t=
he current offsets page.=C2=A0 And as you said, during recovery<br>The star=
tup process should be the only process dirtying offset pages; if<br>a hot s=
tandby reader causes eviction while the lock is released, that<br>should on=
ly help by writing the dirty page out.</div><div><br></div><blockquote clas=
s=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid r=
gb(204,204,204);padding-left:1ex">
&gt; Added both Andrey and Heikki in to-mail, since I&#39;m not sure if thi=
s<br>
&gt; is more extreme than the multixact offset issue we had with 16.12, or =
it<br>
&gt; is at par with that.<br>
<br>
Indeed, let&#39;s wait for at least Heikki&#39;s input.=C2=A0 <br>
<br>
Anyway, for any fixes, I don&#39;t think that it would be a good idea to<br=
>
skip v17 and v18, relying on the SLRU bank locks to not conflict to<br>
bypass the WriteAll() conflict.=C2=A0 Let&#39;s keep all the branches acros=
s<br>
v14~v18 in sync.<br></blockquote><div><br></div><div>Agreed.<br><br>Regards=
,<br>Ayush=C2=A0</div></div></div>

--00000000000084ab730652b44f69--