On Mon, Sep 28, 2026 at 3:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Sat, Sep 26, 2026 at 2:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > It is not clear from comments why it is okay to proceed when
> > remote_slot->restart_lsn > replay_lsn? Because if it is possible to
> > persist the slot in that case then the above issue can hit later say
> > if the promotion happens. I think it is not possible to persist the
> > slot and if that is the case, then we can capture it in comments on
> > the lines: (The check is skipped until replay reaches the remote
...
...
>
> Such a slot won't be persisted before replay catches up, but I think
> the reason is slightly different from what you described, and it
> doesn't wait for a later cycle. When remote_slot->restart_lsn >
> replay_lsn, the check is skipped and we reach
> read_local_xlog_page_guts() via LogicalSlotAdvanceAndCheckSnapState(),
> where we wait for the replay LSN to catch up to the slot's
> confirmed_lsn. It doesn't report "no consistent snapshot". We wait
> there, and the slot can then reach a consistent snapshot and be
> persisted in the same cycle. The patch doesn't change any of this. The
> check on replay_lsn is there so that the new check stays a no-op when
> it
> has nothing to say about the given LSN.
>
> The reason it's okay to proceed is that we created and acquired the
> slot before the wait. If a STATUS_CHANGE record that disables logical
> decoding is replayed while we are waiting, the slot invalidation finds
> our slot, signals a recovery conflict and waits for us to release it
> before invalidating it, and slotsync worker is terminated. So no bad
> slot is left behind.
>
> > If the above reasoning is correct it doesn't seem like a good idea to
> > split the safety of the above mechanism in different functions.
> > Instead, we can move the new check just before
> > update_and_persist_local_synced_slot() and then avoid relying on the
> > code in update_and_persist_local_synced_slot() that can persist the
> > slot.
>
> Does it mean that we return early before calling
> update_and_persist_local_synced_slot() if replay_lsn < restart_lsn? If
> so, I think it would change the existing behavior rather than fix this
> issue.
>
Yeah, so we shouldn't do that but let's update the comment why it is
okay to proceed in that case.
--
With Regards,
Amit Kapila.
