public inbox for [email protected]
help / color / mirror / Atom feedFrom: Amit Kapila <[email protected]>
To: Alexander Korotkov <[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 16:55:53 +0530
Message-ID: <CAA4eK1Ladnm40UO-YC+c7eBYqjyFQSwg0dn1w_CiaMoBTsJUDA@mail.gmail.com> (raw)
In-Reply-To: <CAPpHfdvk-nfu5xzJyPArxWctofWPR+1L9uJ+yyWFA34n6b5rGQ@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>
<CAPpHfdsTiYQPywr4yeqVhUtA+eFdqisqcAOiTb+kiYhi9EVEvw@mail.gmail.com>
<CAPpHfdvk-nfu5xzJyPArxWctofWPR+1L9uJ+yyWFA34n6b5rGQ@mail.gmail.com>
On Tue, May 27, 2025 at 2:48 PM Alexander Korotkov <[email protected]> wrote:
>
> On Tue, May 27, 2025 at 12:12 PM Alexander Korotkov
> <[email protected]> wrote:
> >
> > 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?
>
> Possibly we could implement effective_restart_lsn even for pg18. As I
> know, keeping ABI compatibility is not required for beta.
>
Yeah, we should be able to change ABI during beta, but I can't comment
on the idea of effective_restart_lsn without seeing the patch or a
detailed explanation of this idea.
Now, you see my point related to restart_lsn computation for logical
slots, it is better to also do some analysis of the problem related to
xmin I have highlighted in one of my previous emails [1]. I see your
response to it, but I feel someone needs to give it a try by writing a
test and see the behavior. I am saying because logical slots took
precaution of flushing to disk before updating shared values of xmin
for a reason, whereas similar precautions are not taken for physical
slots, so there could be a problem with that computation as well.
[1]: https://www.postgresql.org/message-id/CAA4eK1KMaPA5jir_SFu%2Bqr3qu55OOdFWVZpuUkqTSGZ9fyPpHA%40mail.g...
--
With Regards,
Amit Kapila.
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: <CAA4eK1Ladnm40UO-YC+c7eBYqjyFQSwg0dn1w_CiaMoBTsJUDA@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