public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Amit Kapila <[email protected]>
Cc: Vitaly Davydov <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected]
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly
Date: Tue, 27 May 2025 12:12:41 +0300
Message-ID: <CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com> (raw)
In-Reply-To: <CAA4eK1Lmd-stXWrNqX9iXugjY=ahhe7zRfyq+-vN512xvF4G2w@mail.gmail.com>
References: <1d12d2-67235980-35-19a406a0@63439497>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<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>
On Tue, May 27, 2025 at 7:08 AM Amit Kapila <[email protected]> wrote:
> On Mon, May 26, 2025 at 10:36 PM Alexander Korotkov
> <[email protected]> wrote:
> >
> > On Mon, May 26, 2025 at 2:43 PM Amit Kapila <[email protected]> wrote:
> > >
> > > On Mon, May 26, 2025 at 3:52 PM Vitaly Davydov <[email protected]> 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 conclude 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
view thread (73+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly
In-Reply-To: <CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox