On Fri, Sep 25, 2026 at 3:39 AM Masahiko Sawada <[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.
I agree with the problem and solution, but I don't think ths slot will later be skipped with 'SS_SKIP_WAL_NOT_FLUSHED' as the WALs are already flushed; it is the replay which is slow and that check compares against GetStandbyFlushRecPtr(), not replay position I think it will wait somewhere in LogicalSlotAdvanceAndCheckSnapState()-->read_local_xlog_page_guts as 'wait_for_wal' is true and standy then waits for replay to happen. If my understanding is correct, slotsync will be stuck on that one slot untli replays happen, but let's see what Nisha has found in her tests. I might be wrong too. --I found that comments 1 and 2 in my previous email about set/reset of 'last_replayed_enable_lsn' are missed to be addressed in v2. --Also v2 does not apply through 'git am'. --I have a suggestion about comment improvement in synchronize_one_slot(), attached the patch. Please incorporate these changes if you agree. thanks Shveta
From d8b4985f88f579942cb1c13e67f146fb32d76ba9 Mon Sep 17 00:00:00 2001 From: Shveta Malik <[email protected]> Date: Fri, 25 Sep 2026 14:48:15 +0530 Subject: [PATCH] comment change --- src/backend/replication/logical/slotsync.c | 51 ++++++++++++---------- 1 file changed, 29 insertions(+), 22 deletions(-) diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index a07118c30a7..715ef6439bc 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -868,32 +868,39 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid, true); /* - * 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 - * deactivation could not invalidate our slot because it did not exist - * yet, and WAL following the 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 remote slot's restart_lsn can predate a status change record + * this standby has already replayed: the primary drops the last + * logical slot, possibly recreating it with the same name, after + * fetch_remote_slots() ran. The resulting deactivation record could + * not invalidate our slot, since it didn't exist yet, so WAL + * following the (stale) remote restart_lsn may lack the information + * logical decoding needs. Checking only whether logical decoding is + * currently enabled is not enough, since it may have been disabled + * and re-enabled since the remote slot information was fetched. * - * 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, so replay cannot get past that record behind our - * back. That is also why the status needs no recheck before the slot - * is persisted. (The invalidation is performed only in hot standby, - * which slot synchronization requires anyway.) + * The check must run after ReplicationSlotCreate(), which makes the + * slot visible and acquired. From this point on, a deactivation + * record 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. * - * WAL beyond the replay position tells us nothing, so a remote - * restart_lsn past it is accepted and left to the interlock above. + * 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. * * 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. + * one, so a slot that would have been usable may still be dropped; + * the next cycle fetches fresh remote information. The slot can't be + * kept in that case, since it would go on using the stale + * restart_lsn. */ replay_lsn = GetXLogReplayRecPtr(NULL); if (remote_slot->restart_lsn <= replay_lsn && -- 2.34.1
