Horton

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

Vitaly Davydov <v.davydov@postgrespro.ru>

View thread
From: "Vitaly Davydov" <v.davydov@postgrespro.ru>
To: "Alexander Korotkov" <aekorotkov@gmail.com>
Cc: "pgsql-hackers" <pgsql-hackers@postgresql.org>, "Masahiko Sawada" <sawada.mshk@gmail.com>, tomas@vondra.me
Date: 2025-05-20T15:44:44Z
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().

Attachments

Hi Alexander,

Thank you very much for the review!

> The patchset doesn't seem to build after 371f2db8b0, which adjusted
> the signature of the INJECTION_POINT() macro.  Could you, please,
> update the patchset accordingly.

I've updated the patch (see attached). Thanks.

> I see in 0004 patch we're calling XLogGetReplicationSlotMinimumLSN()
> before slots synchronization then use it for WAL truncation.
> Generally looks good.  But what about the "if
> (InvalidateObsoleteReplicationSlots(...))" branch?  It calls
> XLogGetReplicationSlotMinimumLSN() again.  Why would the value
> obtained from the latter call reflect slots as they are synchronized
> to the disk?

In patch 0004 I call XLogGetReplicationSlotMinimumLSN() again to keep the old
behaviour - this function was called in KeepLogSeg prior to my change. I also
call CheckPointReplicationSlots at the next line to save the invalidated and
other dirty slots on disk again to make sure, the new oldest LSN is in sync.

The problem I tried to solve in this if-branch is to fix test
src/test/recovery/t/019_replslot_limit.pl which was failed because the WAL was
not truncated enought for the test to pass ok. In general, this branch is not
necessary and we may fix the test by calling checkpoint twice (please, see the
alternative.rej patch for this case). If you think, we should incorporate this
new change, I'm ok to do it. But the WAL will be truncating more lazily.

Furthermore, I think we can save slots on disk right after invalidation, not in
CheckPointGuts to avoid saving invalidated slots twice.

With best regards,
Vitaly