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: Vitaly Davydov <v.davydov@postgrespro.ru>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, tomas@vondra.me
Date: 2025-05-18T21:25:36Z
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().

Hi Vitaly!

On Fri, May 2, 2025 at 8:47 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:
> Thank you for the attention to the patch. I updated a patch with a better
> solution for the master branch which can be easily backported to the other
> branches as we agree on the final solution.
>
> Two tests are introduced which are based on Tomas Vondra's test for logical slots
> with injection points from the discussion (patches: [1], [2], [3]). Tests
> are implemented as module tests in src/test/modules/test_replslot_required_lsn
> directory. I slightly modified the original test for logical slots and created a
> new simpler test for physical slots without any additional injection points.

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 prepared a new solution (patch [4]) which is also based on Tomas Vondra's
> proposal. With a fresh eye, I realized that it can fix the issue as well. It is
> easy and less invasive to implement. The new solution differs from my original
> solution: it is backward compatible (doesn't require any changes in ReplicationSlot
> structure). My original solution can be backward compatible as well if to
> allocate flushed_restart_lsn in a separate array in shmem, not in the
> ReplicationSlot structure, but I believe the new solution is the better one. If
> you still think that my previous solution is the better (I don't think so), I
> will prepare a backward compatible patch with my previous solution.

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?

------
Regards,
Alexander Korotkov
Supabase