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

Hi Alexander,

Thank you for the review. I apologize for a late reply. I missed your email.

> 1) As ReplicationSlotsComputeRequiredLSN() is called each time we need
> to advance the position of WAL needed by replication slots, the usage
> pattern probably could be changed. Thus, we probably need to call
> ReplicationSlotsComputeRequiredLSN() somewhere after change of
> restart_lsn_flushed while restart_lsn is not changed. And probably
> can skip ReplicationSlotsComputeRequiredLSN() in some cases when only
> restart_lsn is changed.

Yes, it is a good idea for investigation, thank you! I guess, It may work for
persistent slots but I'm not sure about other types of slots (ephemeral and
temporary). I have no clear understanding of consequences at the moment. I
propose to postpone it for future, because the proposed changes will me more
invasive.

> 2) I think it's essential to include into the patch test caches which
> fail without patch. You could start from integrating [1] test into
> your patch, and then add more similar tests for different situations.

The problem with TAP tests - it is hard to reproduce without injection points.
The Tomas Vondra's tests create two new injection points. I have to add more
injection points for new tests as well. Injection points help to test the code
but make the code unreadable. I'm not sure, what is the policy of creating
injection points? Personally, I would not like to add new injection points
only to check this particular rare case. I'm trying to create a test without
injection points that should fail occasionally, but I haven't succeeded at
the moment.

I have a question - is there any interest to backport the solution into
existing major releases? I can prepare a patch where restart_lsn_flushed stored
outside of ReplicationSlot structure and doesn't affect the existing API.

With best regards,
Vitaly

On Friday, April 04, 2025 06:22 MSK, Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:

> Hi, Vitaly!
>
> On Mon, Mar 3, 2025 at 5:12 PM Vitaly Davydov <v(dot)davydov(at)postgrespro(dot)ru> wrote:
> > The slot data is flushed to the disk at the beginning of checkpoint. If
> > an existing slot is advanced in the middle of checkpoint execution, its
> > advanced restart LSN is taken to calculate the oldest LSN for WAL
> > segments removal at the end of checkpoint. If the node is restarted just
> > after the checkpoint, the slots data will be read from the disk at
> > recovery with the oldest restart LSN which can refer to removed WAL
> > segments.
> >
> > The patch introduces a new in-memory state for slots -
> > flushed_restart_lsn which is used to calculate the oldest LSN for WAL
> > segments removal. This state is updated every time with the current
> > restart_lsn at the moment, when the slot is saving to disk.
>
> Thank you for your work on this subject. I think generally your
> approach is correct. When we're truncating the WAL log, we need to
> reply on the position that would be used in the case of server crush.
> That is the position flushed to the disk.
>
> While your patch is generality looks good, I'd like make following notes:
>
> 1) As ReplicationSlotsComputeRequiredLSN() is called each time we need
> to advance the position of WAL needed by replication slots, the usage
> pattern probably could be changed. Thus, we probably need to call
> ReplicationSlotsComputeRequiredLSN() somewhere after change of
> restart_lsn_flushed while restart_lsn is not changed. And probably
> can skip ReplicationSlotsComputeRequiredLSN() in some cases when only
> restart_lsn is changed.
> 2) I think it's essential to include into the patch test caches which
> fail without patch. You could start from integrating [1] test into
> your patch, and then add more similar tests for different situations.
>
> Links.
> 1. https://www.postgresql.org/message-id/e3ac0535-e7a2-4a96-9b36-9f765e9cfec5%40vondra.me
>
> ------
> Regards,
> Alexander Korotkov
> Supabase