MIME-Version: 1.0
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>
In-Reply-To: 
 <CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Tue, 27 May 2025 12:18:12 +0300
Message-ID: 
 <CAPpHfdvk-nfu5xzJyPArxWctofWPR+1L9uJ+yyWFA34n6b5rGQ@mail.gmail.com>
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard
 restart unexpectedly
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Vitaly Davydov <v.davydov@postgrespro.ru>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>, tomas@vondra.me
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAPpHfdvk-nfu5xzJyPArxWctofWPR%2B1L9uJ%2ByyWFA34n6b5rGQ%40mail.gmail.com>
Precedence: bulk

On Tue, May 27, 2025 at 12:12=E2=80=AFPM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
>
> On Tue, May 27, 2025 at 7:08=E2=80=AFAM Amit Kapila <amit.kapila16@gmail.=
com> wrote:
> > On Mon, May 26, 2025 at 10:36=E2=80=AFPM Alexander Korotkov
> > <aekorotkov@gmail.com> wrote:
> > >
> > > On Mon, May 26, 2025 at 2:43=E2=80=AFPM Amit Kapila <amit.kapila16@gm=
ail.com> wrote:
> > > >
> > > > On Mon, May 26, 2025 at 3:52=E2=80=AFPM Vitaly Davydov <v.davydov@p=
ostgrespro.ru> wrote:
> > >
> > > > OTOH, if we don't want to adjust physical
> > > > slot machinery, it seems saving the logical slots to disk immediate=
ly
> > > > when its restart_lsn is updated is a waste of effort after your pat=
ch,
> > > > no? If so, why are we okay with that?
> > >
> > > I don't think so.  I think the reason why logical slots are synced to
> > > disk immediately after update is that logical changes are not
> > > idempotent (you can't safely apply the same change twice) unlike
> > > physical block-level changes.  This is why logical slots need to be
> > > synced to prevent double replication of same changes, which could
> > > lead, for example, to double insertion.
> > >
> >
> > Hmm, if this has to be true, then even in the else branch of
> > LogicalConfirmReceivedLocation [1], we should have saved the slot.
> > AFAIU, whether the logical changes are sent to the client is decided
> > based on two things: (a) the replication origins, which tracks
> > replication progress and are maintained by clients (which for built-in
> > replication are subscriber nodes), see [2]; and (b) confirmed_flush
> > LSN maintained in the slot by the server. Now, for each ack by the
> > client after applying/processing changes, we update the
> > confirmed_flush LSN of the slot but don't immediately flush it. This
> > shouldn't let us send the changes again because even if the system
> > crashes and restarts, the client will send the server the location to
> > start sending the changes from based on its origin tracking. There is
> > more to it, like there are cases when confirm_flush LSN in the slot
> > could be ahead the origin's LSN, and we handle all such cases, but I
> > don't think those are directly related here, so I am skipping those
> > details for now.
> >
> > Note that LogicalConfirmReceivedLocation won't save the slot to disk
> > if it updates only confirmed_flush LSN, which is used to decide
> > whether to send the changes.
>
> You're right, I didn't study these aspects careful enough.
>
> > > LogicalConfirmReceivedLocation() implements immediate sync for
> > > different reasons.
> > >
> >
> > I may be missing something, but let's discuss some more before we concl=
ude this.
>
> So, yes probably LogicalConfirmReceivedLocation() tries to care about
> keeping all WAL segments after the synchronized value of restart_lsn.
> But it just doesn't care about concurrent
> ReplicationSlotsComputeRequiredLSN().  In order to fix that logic, we
> need effective_restart_lsn field by analogy to effective_catalog_xmin
> (similar approach was discussed in this thread before).  But that
> would require ABI compatibility breakage.
>
> So, I'd like to propose following: backpatch 0001 and 0002, but
> implement effective_restart_lsn field for pg19.  What do you think?

Possibly we could implement effective_restart_lsn even for pg18.  As I
know, keeping ABI compatibility is not required for beta.

------
Regards,
Alexander Korotkov
Supabase