From: "Vitaly Davydov" <v.davydov@postgrespro.ru>
In-Reply-To: 
 <CAPpHfdvxqjE+RZdoOODQZAmQip6NpvSH40DuvVFWuf_00ek-yQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
References: <1d12d2-67235980-35-19a406a0@63439497>
 <683f3c82-d38e-436d-88fd-27722af62005@vondra.me>
 <8679fea7-94ce-4a52-8e48-1a8cd0857fcb@vondra.me>
 <ec90c6ed-92eb-4fa5-861b-af6167f04c43@vondra.me>
 <e3ac0535-e7a2-4a96-9b36-9f765e9cfec5@vondra.me>
 <135e8-675c4600-21-304d7f40@228322535>
 <1538a2-67c5c700-7-77ec5a80@179382871>
 <CAPpHfdvQPndPMbPvrHqUYbi_HnTZxxaZ4cafvF+9Mx56v07W-w@mail.gmail.com>
 <27589a-680a4b80-6d-6e07880@117064404>
 <CAPpHfdujSUM6de2ZD6gP_3vjUonHgiqh7StPZ5AQz-=4-qhuzA@mail.gmail.com>
 <CAD21AoAbP8NJN3UZXA=RMB_LMwWFDwkV6Erzf7WpAa=jqs0_6A@mail.gmail.com>
 <CAPpHfdtgwhFFpb8v8x2Humdj3F_DjaeBWY_h20g-Owh5tMArCA@mail.gmail.com>
 <CAD21AoDp4Zabk49G94TdWrthfv+0mhHLA6yw-AJxxRSFWPep+g@mail.gmail.com>
 <3ea6a1-68150580-1-7b53be0@80017081>
 <CAPpHfdvpcboCAxbj+xPmCWNRZxG=JwW3qRsz4No_OvP3j03ERg@mail.gmail.com>
 <309f08-682ca380-25-139bf240@62751029>
 <CAPpHfduDLCrQ=vp1KBJu5fnSzgQp9ebGRV7i2poHK9VthwaL7g@mail.gmail.com>
 <CAPpHfdutKQxpm-gJgiZRb2ouKC9+HZx3fG3F00zd=xdxDidm_g@mail.gmail.com>
 <CAA4eK1+eebaviZ9TCU4DHAOZzRRbWe0HgdjjQ1VVu7P_pEsiYQ@mail.gmail.com>
 <CAPpHfdsk9_mrZaR_twwe6ntFUYCwu9SM1t4n6SmWNTZDZGefoQ@mail.gmail.com>
 <CAA4eK1KMaPA5jir_SFu+qr3qu55OOdFWVZpuUkqTSGZ9fyPpHA@mail.gmail.com>
 <2c1d-68344100-b7-3411b8c0@256938178>
 <CAA4eK1LmVoZ8jm5Jt3DERN9Z=y6L6xj5O0asw0smWw31Y8KfVw@mail.gmail.com>
 <CAPpHfdtp61pSt72KxxOnnV_Oj4OTAiKYCSTek0Pm87hVDod4Bg@mail.gmail.com>
 <CAA4eK1Lmd-stXWrNqX9iXugjY=ahhe7zRfyq+-vN512xvF4G2w@mail.gmail.com>
 <CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com>
 <CAPpHfdvk-nfu5xzJyPArxWctofWPR+1L9uJ+yyWFA34n6b5rGQ@mail.gmail.com>
 <CAA4eK1Ladnm40UO-YC+c7eBYqjyFQSwg0dn1w_CiaMoBTsJUDA@mail.gmail.com>
 <CAPpHfduX--KKX853UwaJ8Cjo5bxbh19V_+McKQ3p9aS6b8T1Wg@mail.gmail.com>
 <CAA4eK1+VbBxYY-rG4B7Jq6TsYvdjt3d2nbUrs7p9-1sP5mH+sw@mail.gmail.com>
 <CAPpHfdvxqjE+RZdoOODQZAmQip6NpvSH40DuvVFWuf_00ek-yQ@mail.gmail.com>
Date: Thu, 05 Jun 2025 18:21:20 +0300
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>,
 "Masahiko Sawada" <sawada.mshk@gmail.com>, tomas@vondra.me
To: "Alexander Korotkov" <aekorotkov@gmail.com>,
 "Amit Kapila" <amit.kapila16@gmail.com>
MIME-Version: 1.0
Message-ID: <1d97ec-6841b600-3-b7b4780@40747246>
Subject: =?utf-8?q?Re=3A?= Slot's =?utf-8?q?restart=5Flsn?= may point to
 removed WAL
 segment after hard restart unexpectedly
User-Agent: SOGoMail 5.12.0
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/1d97ec-6841b600-3-b7b4780%4040747246>
Precedence: bulk

Dear Alexander, Amit

Alexander Korotkov wrote:
> Also, I've changed ReplicationSlotsComputeRequiredLSN() call to
> CheckPointReplicationSlots() to update required LSN after
> SaveSlotToPath() updated last=5Fsaved=5Frestart=5Flsn.  This helps to=
 pass
> checks in 001=5Fstream=5Frep.pl without additional hacks.

Thank you for the improvement and patch preparation. I confirm the test=
 is
passed without additional hacks now.

I sill do not understand why this solution is favored. It is, in my opi=
nion,
a non backward-compatible solution. In any case, I'm ok to go with this=
 patch.
If needed, I may prepare a backward-compatible solution where
last=5Fsaved=5Frestart=5Flsn values will be in an another place of the =
shmem, rather
than in ReplicationSlot struct.

I still would like to add my 5 cents to the discussion.

The purpose of the xmin value is to prevent tuples from vacuuming. Slot=
s'
restart=5Flsn values are used to calculate the oldest lsn to keep WAL s=
egments
from removal in checkpoint. These processes are pretty independent.

The logical slots are advanced in 2 steps. At the first step, the logic=
al
decoding stuff periodically sets consistent candidate values for catalo=
g=5Fxmin and
restart=5Flsn. At the second step, when LogicalConfirmReceivedLocation =
is called,
the candidate values are assigned on catalog=5Fxmin and restart=5Flsn v=
alues based
on the confirmed lsn value. The slot is saved with these consistent val=
ues.
It is important, that the candidate values are consistent, decoding gua=
rantees
it. In case of a crash, we should guarantee that the loaded from the di=
sk
catalog=5Fxmin and restart=5Flsn values are consistent and valid for lo=
gical slots.
LogicalConfirmReceivedLocation function keeps this consistency by updat=
ing them
from consistent candidate values in a single operation.

We have to guarantee that we use saved to disk values to calculate xmin=
 horizon
and slots' oldest lsn. For this purpose, effective=5Fcatalog=5Fxmin is =
used. We
update effective=5Fcatalog=5Fxmin in LogicalConfirmReceivedLocation jus=
t
after saving slot to disk. Another place where we update effective=5Fca=
talog=5Fxmin
is when walsender receives hot standby feedback message.

Once, we have two independent processes (vacuuming, checkpoint), we can=
 calculate
xmin horizon and oldest WAL lsn values independently (at different time=
s) from
the saved to disk values. Note, these values are updated in a non atomi=
c way.

The xmin value is set when the node receives hot standby feedback and i=
t is used
to keep tuples from vacuuming as well as catalog=5Fxmin for decoding st=
uff. Not
sure, xmin is applicable for logical replication.

The confirmed flush lsn is used as a startpoint when a peer node doesn'=
t provide
the start lsn and to check that the start lsn is not older than the lat=
est
confirmed flush lsn. The saving of the slot on disk at each call of
LogicalConfirmReceivedLocation doesn't help to avoid conflicts complete=
ly, but
it helps to decrease the probability of conflicts. So, i'm still not su=
re, we
need to save logical slots on each advance to avoid conflicts, because =
it
doesn't help in general. The conflicts should be resolved by other mean=
s.

Once, we truncate old wal segments in checkpoint only. I believe, it is=
 ok if we
calculate the oldest lsn only at the beginning of the checkpoint, as it=
 was in
the alternative solution. I think, we can update xmin horizon in checkp=
oint only
but the horizon advancing will be more lazy in this case.

Taking into account these thoughts, I can't see any problems with the a=
lternative
patch where oldest wal lsn is calculated only in checkpoint.

With best regards,
Vitaly