On Fri, Sep 25, 2026 at 3:39 AM Masahiko Sawada <[email protected]> wrote: > > On Thu, Sep 24, 2026 at 4:41 AM Nisha Moond <[email protected]> wrote: > > > > On Wed, Sep 23, 2026 at 1:45 AM Masahiko Sawada <[email protected]> > > wrote: > > > > > > On Mon, Sep 21, 2026 at 11:32 PM Nikolay Samokhvalov <[email protected]> > > > wrote: > > > > > > > > On Thu, Jul 16, 2026 at 6:52 AM Masahiko Sawada > > > > <[email protected]> wrote: > > > > > For slot synchronization, the local slot could be created and > > > > > persisted based on the remote slot information fetched before the > > > > > deactivation was replayed, leaving a valid slot whose restart_lsn > > > > > precedes the deactivation. Decoding such a slot after a failover fails > > > > > with: > > > > > > > > > > ERROR: unexpected logical decoding status change 0 > > > > > > > > > > These races are confined to the narrow window between checking the > > > > > logical decoding status and the new slot becoming visible; once the > > > > > slot is visible, the invalidation performed by the deactivation > > > > > already covers it. So the fix is simple: re-check the logical decoding > > > > > status after the new slot becomes visible. Regular slot creation > > > > > raises an error and slot synchronization skips persisting the slot. If > > > > > the deactivation happens after the recheck instead, it is guaranteed > > > > > to invalidate the now-visible slot as usual. The attached 0002 > > > > > implements this. > > > > > > > > The disable/re-enable case described in the comment above the final > > > > IsLogicalDecodingEnabled() check in > > > > update_and_persist_local_synced_slot() is > > > > reachable. > > > > > > > > On b73d13c3, the reproducer uses this sequence: > > > > > > > > 1. Slot sync fetches failover slot S and pauses at > > > > replication-slot-create-begin, before creating the local slot. > > > > 2. The primary drops S. The standby replays the logical-decoding > > > > deactivation while no local S exists to invalidate. > > > > 3. The primary recreates S. The standby replays the reactivation. > > > > 4. The old slot sync resumes with the first incarnation's restart_lsn. > > > > > > > > The final IsLogicalDecodingEnabled() check now returns true, so the old > > > > slot > > > > information is persisted. After promoting the standby, decoding that > > > > slot > > > > fails with: > > > > > > > > ERROR: unexpected logical decoding status change 0 > > > > > > Thank you for the report. Yes, while the window is very short in > > > practice, it indeed happens if the logical decoding is disabled and > > > re-enabled (by dropping and creating the same name failover slot) > > > between the slotsync worker fetches the slot information and creates > > > it. > > > > > > It actually hits my concern mentioned in the comment in > > > update_and_persist_local_synced_slot(): > > > > > > * XXX: this check cannot detect the case where logical decoding is > > > * already re-enabled by a slot creation on the primary at this point. > > > * Detecting that would require comparing the slot's restart_lsn with > > > the > > > * LSN at which logical decoding was last enabled. > > > > > > > The attached patch adds a logical-decoding status generation. Slot sync > > > > records it before fetching remote slot information and refuses to > > > > persist a > > > > new slot if the generation changed in the meantime. It drops the > > > > temporary > > > > slot so that the next attempt fetches the current incarnation. > > > > > > Thank you for the patch. > > > > > > An alternative approach that I think is better is to have the LSN of > > > the last replayed status change record in LogicalDecodingCtlData, and > > > check if logical decoding has been enabled since the remote slot's > > > restart_lsn. That's simpler than the proposed approach as we don't > > > need to increment the generation counter at both activation and > > > deactivation (which is not necessary outside recovery), nor to add > > > logical_decoding_generation to RemoteSlot. It also checks what we > > > actually need, that is, whether the WAL from the restart_lsn can be > > > decoded, rather than whether the status changed while synchronizing > > > slots. > > > > > > Also, I think it's better to move the check to right after > > > ReplicationSlotCreate() in synchronize_one_slot() because (1) it can > > > simplify the code flow as we don't need to care about the slot dropped > > > in update_and_persist_local_synced_slot(), (2) it can save the WAL > > > reservation and the xmin_horizon computation, and (3) IIUC with the > > > proposed patch, the check can be bypassed when > > > update_and_persist_local_synced_slot() returns early due to > > > slotsync_skip_reason, leaving a temporary slot with the stale > > > restart_lsn. Once the slot passes the check right after its creation, > > > a later deactivation invalidates the slot, so we don't need to check > > > it again before persisting the slot. > > > > > > I've attached the patch. > > > > > > > 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. >
This solution is better than my suggestion. I tested v2 for both the original problem and the standby-lagging case, and it works as expected. In the lagging case, the slot now remains in the temporary state and is not recreated in a loop. > > > > 2) The overview comment at the top of slotsync.c explains each reason > > a slot isn't synced yet, but it doesn't mention this new > > drop-and-retry case. Should we add it there too? > > The patch describes the details of the disabled/re-enabled case in > synchronize_one_slot() and it looks sufficient to me. > Okay. > I've updated the patch for the above idea. In this version, the patch > tracks only the last STATUS_CHANGE record that enables logical > decoding, which makes it easy to check if logical decoding has > continuously been enabled since the remote slot's restart_lsn. > Overall, the patch LGTM. I have no further comments. -- Thanks, Nisha
