On Sat, Sep 26, 2026 at 2:44 PM Amit Kapila <[email protected]> wrote: > > On Fri, Sep 25, 2026 at 6:25 PM Masahiko Sawada <[email protected]> wrote: > > > > I've incorporated your comment suggestions, and updated cosmetic > > things.Please review them. > > > > + /* > + * The remote slot information can predate a status change record that > + * this standby has already replayed. That happens when the last > + * logical slot on the primary is dropped, and possibly re-created > + * with the same name, after fetch_remote_slots() ran. The resulting > + * deactivation could not invalidate our slot because it did not exist > + * yet, and WAL following the (stale) remote restart_lsn may lack the > + * information logical decoding needs. Checking only whether logical > + * decoding is enabled is not enough, as it can have been disabled and > + * enabled again in the meantime. > + * > + * The check has to come after ReplicationSlotCreate(), which makes > + * the slot both visible and acquired. A deactivation replayed from > + * here on finds the slot in InvalidatePossiblyObsoleteSlot(), signals > + * a recovery conflict and waits for the slot to be released before > + * invalidating it (only in hot standby, which slot synchronization > + * requires anyway). Replay therefore cannot get past that record > + * behind our back, so the slot never needs to be rechecked before > + * being persisted. > + * > + * The check only runs once replay has reached the remote restart_lsn; > + * otherwise it is skipped and the slot is kept as-is. Without this, a > + * standby lagging behind the primary (replay paused, or a large > + * recovery_min_apply_delay) could fetch a live, valid restart_lsn > + * from the primary and have it rejected by > + * StandbyLogicalDecodingEnabledSince(), whose answer reflects only > + * WAL replayed so far and says nothing about an LSN replay hasn't > + * reached yet. That would drop a perfectly good slot every cycle. > + * > + * Even so, the comparison uses the remote restart_lsn rather than the > + * local one, so a slot that would have been usable may be dropped; > + * the next cycle fetches fresh information. The slot cannot be kept, > + * as it would go on using the stale restart_lsn. > + */ > + replay_lsn = GetXLogReplayRecPtr(NULL); > + if (remote_slot->restart_lsn <= replay_lsn && > + !StandbyLogicalDecodingEnabledSince(remote_slot->restart_lsn)) > + { > + ereport(LOG, > + errmsg("could not synchronize replication slot \"%s\"", > + remote_slot->name), > + errdetail("Logical decoding was disabled after the remote slot's > restart LSN %X/%08X.", > + LSN_FORMAT_ARGS(remote_slot->restart_lsn))); > + > + ReplicationSlotDropAcquired(false); > + > + if (slot_persistence_pending) > + *slot_persistence_pending = true; > + > + return false; > + } > + > /* For shorter lines. */ > slot = MyReplicationSlot; > > 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 > restart_lsn, as StandbyLogicalDecodingEnabledSince() describes > replayed WAL only and would otherwise reject a valid restart_lsn from > a lagging standby. Skipping it lets no bad slot through, as the slot > is not persisted in this cycle anyway. It starts out with the remote > restart_lsn, see reserve_wal_for_local_slot(), and > update_local_synced_slot() can neither find a serialized snapshot at > an LSN this standby has not replayed nor build one by decoding WAL it > has not replayed. It therefore reports no consistent snapshot, the > slot stays temporary, and a later cycle retries it once replay has > advanced.).
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. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
