Hi, On Fri, Sep 25, 2026 at 6:09 AM Masahiko Sawada <[email protected]> wrote: > > On Thu, Sep 24, 2026 at 4:41 AM Nisha Moond <[email protected]> wrote: > > > > I tested the patch and it fixes the problem. I found no critical > > issues. A couple of comments: > > 1) Now that a newly created synced slot is dropped on a failed new > > check rather than kept as RS_TEMPORARY, a standby that is lagging in > > replay can end up creating and dropping the slot on every sync cycle. > > For example, replay is paused with pg_wal_replay_pause() or > > recovery_min_apply_delay is large. After the primary turns logical > > decoding off and then on again, the standby receives the activation > > record but doesn't replay it. Meanwhile the slotsync worker keeps > > fetching the failover slot, creates it, fails the new > > IsLogicalDecodingEnabledSince() check, and drops it. This repeats > > every cycle until the record is replayed. > > > > Each cycle creates the slot on disk and a pgstat entry, then removes > > both again. I think this can be avoided with a cheaper pre-check, > > IsLogicalDecodingEnabledSince(remote_slot->restart_lsn), before > > ReplicationSlotCreate(). > > > > Thoughts? > > I agree with your analysis. I think that in this case, the logical > slot doesn't need to be dropped because WAL records after its > restart_lsn are written with logical decoding information. Thinking on > IsLogicalDecodingEnabledSince() further, I think it can work fine for > the slot only when the replay LSN >= slot's restart_lsn. If the slot's > restart_lsn > replay_lsn, we can leave the slot. Such a slot will be > skipped for SS_SKIP_WAL_NOT_FLUSHED anyway. That way, the slot would > have to be recreated only in the disable/re-enable case.
The patch looks good to me overall, but I have a question for the check of replay position: + replay_lsn = GetXLogReplayRecPtr(NULL); + if (remote_slot->restart_lsn <= replay_lsn && If the goal is to avoid retrying when the standby is lagging, couldn't we simply avoid setting slot_persistence_pending = true in this case? The original slot is dropped anyway, and any slot re-created later is a different one. This seems more similar to the SS_SKIP_INVALID case, where we don't retry even if a re-created slot exists. I think that would also make the logic simpler. Best Regards, Zhijie Hou
