Horton

Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Vitaly Davydov <v.davydov@postgrespro.ru>, vignesh C <vignesh21@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Tom Lane <tgl@sss.pgh.pa.us>, Alexander Lakhin <exclusion@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, "tomas@vondra.me" <tomas@vondra.me>
Date: 2025-06-20T00:18:20Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Update comment for ReplicationSlot.last_saved_restart_lsn

  2. Fix CheckPointReplicationSlots() with max_replication_slots == 0

  3. Remove excess assert from InvalidatePossiblyObsoleteSlot()

  4. Improve runtime and output of tests for replication slots checkpointing.

  5. Keep WAL segments by slot's last saved restart LSN

  6. Fix an assert in CheckPointReplicationSlots().

On Thu, Jun 19, 2025 at 1:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Jun 18, 2025 at 10:17 PM Alexander Korotkov
> <aekorotkov@gmail.com> wrote:
> >
> > On Wed, Jun 18, 2025 at 6:50 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:
> > > > I think, it is a good idea. Once we do not use the generated data, it is ok
> > > > just to generate WAL segments using the proposed function. I've tested this
> > > > function. The tests worked as expected with and without the fix. The attached
> > > > patch does the change.
> > >
> > > Sorry, forgot to attach the patch. It is created on the current master branch.
> > > It may conflict with your corrections. I hope, it could be useful.
> >
> > Thank you.  I've integrated this into a patch to improve these tests.
> >
> > Regarding assertion failure, I've found that assert in
> > PhysicalConfirmReceivedLocation() conflicts with restart_lsn
> > previously set by ReplicationSlotReserveWal().  As I can see,
> > ReplicationSlotReserveWal() just picks fresh XLogCtl->RedoRecPtr lsn.
> > So, it doesn't seems there is a guarantee that restart_lsn never goes
> > backward.  The commit in ReplicationSlotReserveWal() even states there
> > is a "chance that we have to retry".
> >
>
> I don't see how this theory can lead to a restart_lsn of a slot going
> backwards. The retry mentioned there is just a retry to reserve the
> slot's position again if the required WAL is already removed. Such a
> retry can only get the position later than the previous restart_lsn.

Yes, if retry is needed, then the new position must be later for sure.
What I mean is that ReplicationSlotReserveWal() can reserve something
later than what standby is going to read (and correspondingly report
with PhysicalConfirmReceivedLocation()).

> >  Thus, I propose to remove the
> > assertion introduced by ca307d5cec90.
> >
>
> If what I said above is correct, then the following part of the commit
> message will be incorrect:
> "As stated in the ReplicationSlotReserveWal() comment, this is not
> always true. Additionally, this issue has been spotted by some
> buildfarm
> members."

I agree, this comment needs improvement in terms of clarity.

Meanwhile I've pushed the patch for TAP tests, which I think didn't
get any objections.

------
Regards,
Alexander Korotkov
Supabase