On Thu, Sep 24, 2026 at 2:19 AM shveta malik <[email protected]> wrote:
>
> 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:
Thank you for reviewing the patch!
> 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()?
Agreed.
>
> 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.
Agreed.
>
> 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
Okay.
>
> 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);
I think that it doesn't cover the case where we replay the
STATUS_CHANGE record with logical_decoding=true while the logical
decoding is enabled. IIUC this can technically happen since we start
the recovery from a redo point but the initial logical decoding status
is based on the checkpoint record.
> 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.
Agreed.
>
> 6)
> IsLogicalDecodingEnabledSince() is meaningful only on standby, shall
> we add Assert(RecoveryInProgress())?
Agreed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
