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>
In-Reply-To: 
 <CAA4eK1Lmd-stXWrNqX9iXugjY=ahhe7zRfyq+-vN512xvF4G2w@mail.gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Tue, 27 May 2025 12:12:41 +0300
Message-ID: 
 <CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@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/CAPpHfdsTiYQPywr4yeqVhUtA%2BeFdqisqcAOiTb%2BkiYhi9EVEvw%40mail.gmail.com>
Precedence: bulk

On Tue, May 27, 2025 at 7:08=E2=80=AFAM Amit Kapila <amit.kapila16@gmail.co=
m> 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@gmai=
l.com> wrote:
> > >
> > > On Mon, May 26, 2025 at 3:52=E2=80=AFPM Vitaly Davydov <v.davydov@pos=
tgrespro.ru> wrote:
> >
> > > OTOH, if we don't want to adjust physical
> > > slot machinery, it seems saving the logical slots to disk immediately
> > > when its restart_lsn is updated is a waste of effort after your patch=
,
> > > 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 conclud=
e 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?

------
Regards,
Alexander Korotkov
Supabase