Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly - Mailing list pgsql-hackers

On Mon, May 26, 2025 at 2:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, May 26, 2025 at 3:52 PM Vitaly Davydov <v.davydov@postgrespro.ru> 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