Re: Fix "unexpected logical decoding status change" error; from concurrent logical decoding activation - Mailing list pgsql-hackers

On Sat, Sep 26, 2026 at 3:54 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Sep 25, 2026 at 2:28 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Sep 25, 2026 at 3:39 AM Masahiko Sawada <sawada.mshk@gmail.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.
> >
> > 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.
>
> You're right, if only the replay is delayed, it can end up waiting in
> read_local_xlog_page_guts(). But I think this is pre-existing behavior
> for a remote slot whose confirmed_lsn is ahead of the standby's replay
> position.

Yes. I agree.

>
> On the other hand, if the WAL has not even been flushed on the standby
> yet, we skip the slot with SS_SKIP_WAL_NOT_FLUSHED before reaching
> that point. The temporary slot is kept and we retry in the next cycle,
> so we don't get stuck there.

Right.

> >
> > --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.
>
> Sorry I forgot to mention about comment 1 and 2; since the updated
> patch renamed the field name to last_replayed_enable_lsn I think we
> don't necessarily need to reset it at
> UpdateLogicalDecodingStatusEndOfRecovery().

Okay. Works for me.

> Also it renamed the
> function name to StandbyLogicalDecodingEnabledSince() so it makes
> sense to me to leave the field. As for comment 2, I think it's better
> to have an shmem-init function for LogicalDecodingCtl rather than
> initializing the new field in StartupLogicalDecodingStatus(). So I
> prepared a patch for that (0001 patch).

Okay, makes sense.

> I've incorporated your comment suggestions, and updated cosmetic
> things.Please review them.
>

Okay, looks good. I can have a look again once Amit's suggestion is
incorporated.

thanks
Shveta