On Mon, Sep 28, 2026 at 3:49 PM Masahiko Sawada <[email protected]> wrote: > > On Sat, Sep 26, 2026 at 2:44 PM Amit Kapila <[email protected]> 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.
