Re: Fix "unexpected logical decoding status change" error; from concurrent logical decoding activation

On Fri, Sep 25, 2026 at 6:21 AM Zhijie Hou <houzhijie22(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Fri, Sep 25, 2026 at 6:09 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Sep 24, 2026 at 4:41 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> 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.

I think the point is that StandbyLogicalDecodingEnabledSince() cannot
answer for an LSN that the standby hasn't replayed yet. If restart_lsn
is ahead of the replay LSN, we would end up answering "no" in this
case and dropping the slot unnecessarily even if the primary already
enables and has a valid logical slot. This would be repeated until the
standby replayed the STATUS_CHANGE record that enables logical
decoding.

I don't think SS_SKIP_INVALID is quite the same situation. There the
remote slot is really invalidated and there is nothing to wait for,
whereas here the remote slot is healthy and becomes syncable as soon
as replay catches up.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com