On Wed, Sep 23, 2026 at 1:46 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 reporting the issue.
> 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.
I agree.
> Also, I think it's better to move the check to right after
> ReplicationSlotCreate() in synchronize_one_slot()
I also think so.
> 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.
I agree on pt 1 and 2, need to verify 3 though.
> 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 agree. It is not only invalidated but will be dropped too by
drop_local_obsolete_slots() at the start of the next sync-cycle.
> I've attached the patch.
>
Thanks for the patch. I like this approach and it looks good overall.
I'm still in the process of verifying it, but here are a few comments
in the meantime:
1)
Comment atop status_change_end_lsn says "only maintained during
recovery", but after promotion the stale value stays in shared memory.
Shall we set it to InvalidXLogRecPtr in
UpdateLogicalDecodingStatusEndOfRecovery()?
2)
Shall we set status_change_end_lsn to InvalidXLogRecPtr explicitly in
StartupLogicalDecodingStatus(), before the early return for wal_level
= minimal? That makes the intention clear rather than relying on
zero-initialized shared memory.
3)
Shall we say in a comment(atop the field or/and in
IsLogicalDecodingEnabledSince()) that the new field is not persisted,
and why that is safe
4)
The comment atop SetLogicalDecodingStatusChangeLSN() says it must be
called before the record changes the status. Why don't we make the
update part of EnableLogicalDecoding() itself? Something like:
EnableLogicalDecoding(XLogRecPtr redo_end_lsn)
{
...
in_recovery = RecoveryInProgress();
/* Invalid unless called from redo */
Assert(in_recovery == XLogRecPtrIsValid(redo_end_lsn));
..
LWLockAcquire(LogicalDecodingControlLock, LW_EXCLUSIVE);
...
if (in_recovery)
LogicalDecodingCtl->last_enabled_end_lsn = redo_end_lsn; /* same lock hold */
else
write_logical_decoding_status_update_record(true);
5)
+ ereport(LOG,
+ errmsg("could not synchronize replication slot \"%s\"",
+ remote_slot->name),
+ errdetail("Logical decoding on the standby has not been continuously
enabled since the remote slot's restart LSN %X/%08X.",
+ LSN_FORMAT_ARGS(remote_slot->restart_lsn)));
Instead of above, shall we say something like:
Logical decoding was disabled after the remote slot's restart LSN %X/%08X
Or
Logical decoding status changed after the remote slot's restart LSN %X/%08X.
6)
IsLogicalDecodingEnabledSince() is meaningful only on standby, shall
we add Assert(RecoveryInProgress())?
thanks
Shveta
