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: Mon, 26 May 2025 20:06:23 +0300
Message-ID: <CAPpHfdtp61pSt72KxxOnnV_Oj4OTAiKYCSTek0Pm87hVDod4Bg@mail.gmail.com> (raw)
In-Reply-To: <CAA4eK1LmVoZ8jm5Jt3DERN9Z=y6L6xj5O0asw0smWw31Y8KfVw@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>
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:
> >
> > Dear Alexander, Amit, All
> >
> > > Amit wrote:
> > > > Is my understanding correct that we need 0001 because
> > > > PhysicalConfirmReceivedLocation() doesn't save the slot to disk after
> > > > changing the slot's restart_lsn?
> > >
> > > Yes. Also, even if it would save slot to the disk, there is still
> > > race condition that concurrent checkpoint could use updated value from
> > > the shared memory to clean old WAL segments, and then crash happens
> > > before we managed to write the slot to the disk.
> > >
> > > How can that happen, if we first write the updated value to disk and
> > > then update the shared memory as we do in
> > > LogicalConfirmReceivedLocation?
> >
> > I guess, that the problem with logical slots still exist. Please, see the tap
> > test: src/test/recovery/t/046_logical_slot.pl from the v6 version of the patch.
> > A race condition may happen when logical slot's restart_lsn was changed but not
> > yet written to the disk. Imagine, there is another physical slot which is
> > advanced at this moment. It recomputes oldest min LSN and takes into account
> > changed but not saved to disk restart_lsn of the logical slot. We come to the
> > situation when the WAL segment for the logical slot's restart_lsn may be
> > truncated after immediate restart.
> >
>
> Okay, so I was missing the point that the physical slots can consider
> the updated value of the logical slot's restart_lsn. The point I was
> advocating for logical slots sanctity was when no physical slots are
> involved. When updating replicationSlotMinLSN value in shared memory,
> the logical slot machinery took care that the value we use should be
> flushed to disk. One can argue that we should improve physical slots
> machinery so that it also takes care to write the slot to disk before
> updating the replicationSlotMinLSN, which is used to remove WAL. I
> understand that the downside is physical slots will be written to disk
> with a greater frequency, which will not be good from the performance
> point of view, but can we think of doing it for the period when a
> checkpoint is in progress?
That could cause replication slowdown while checkpointing is
in-progress. This is certainly better than slowing down the
replication permanently, but still doesn't look good.
> 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.
> I understand that your proposed patch fixes the reported problem but I
> am slightly afraid that the proposed solution is not a good idea w.r.t
> logical slots so I am trying to see if there are any other alternative
> ideas to fix this issue.
I don't understand exact concerns about this fix. For sure, we can
try to implement a fix hacking LogicalConfirmReceivedLocation() and
PhysicalConfirmReceivedLocation(). But that would be way more
cumbersome, especially if we have to keep ABI compatibility. Also, it
doesn't seem to me that either LogicalConfirmReceivedLocation() or
PhysicalConfirmReceivedLocation() currently try to address this issue:
LogicalConfirmReceivedLocation() implements immediate sync for
different reasons.
------
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: <CAPpHfdtp61pSt72KxxOnnV_Oj4OTAiKYCSTek0Pm87hVDod4Bg@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