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

On Fri, Sep 25, 2026 at 6:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I've incorporated your comment suggestions, and updated cosmetic
> things.Please review them.
>

+ /*
+ * The remote slot information can predate a status change record that
+ * this standby has already replayed. That happens when the last
+ * logical slot on the primary is dropped, and possibly re-created
+ * with the same name, after fetch_remote_slots() ran. The resulting
+ * deactivation could not invalidate our slot because it did not exist
+ * yet, and WAL following the (stale) remote restart_lsn may lack the
+ * information logical decoding needs. Checking only whether logical
+ * decoding is enabled is not enough, as it can have been disabled and
+ * enabled again in the meantime.
+ *
+ * The check has to come after ReplicationSlotCreate(), which makes
+ * the slot both visible and acquired. A deactivation replayed from
+ * here on finds the slot in InvalidatePossiblyObsoleteSlot(), signals
+ * a recovery conflict and waits for the slot to be released before
+ * invalidating it (only in hot standby, which slot synchronization
+ * requires anyway). Replay therefore cannot get past that record
+ * behind our back, so the slot never needs to be rechecked before
+ * being persisted.
+ *
+ * The check only runs once replay has reached the remote restart_lsn;
+ * otherwise it is skipped and the slot is kept as-is. Without this, a
+ * standby lagging behind the primary (replay paused, or a large
+ * recovery_min_apply_delay) could fetch a live, valid restart_lsn
+ * from the primary and have it rejected by
+ * StandbyLogicalDecodingEnabledSince(), whose answer reflects only
+ * WAL replayed so far and says nothing about an LSN replay hasn't
+ * reached yet. That would drop a perfectly good slot every cycle.
+ *
+ * Even so, the comparison uses the remote restart_lsn rather than the
+ * local one, so a slot that would have been usable may be dropped;
+ * the next cycle fetches fresh information. The slot cannot be kept,
+ * as it would go on using the stale restart_lsn.
+ */
+ replay_lsn = GetXLogReplayRecPtr(NULL);
+ if (remote_slot->restart_lsn <= replay_lsn &&
+ !StandbyLogicalDecodingEnabledSince(remote_slot->restart_lsn))
+ {
+ ereport(LOG,
+ errmsg("could not synchronize replication slot \"%s\"",
+   remote_slot->name),
+ errdetail("Logical decoding was disabled after the remote slot's
restart LSN %X/%08X.",
+  LSN_FORMAT_ARGS(remote_slot->restart_lsn)));
+
+ ReplicationSlotDropAcquired(false);
+
+ if (slot_persistence_pending)
+ *slot_persistence_pending = true;
+
+ return false;
+ }
+
  /* For shorter lines. */
  slot = MyReplicationSlot;

It is not clear from comments why it is okay to proceed when
remote_slot->restart_lsn > replay_lsn? Because if it is possible to
persist the slot in that case then the above issue can hit later say
if the promotion happens. I think it is not possible to persist the
slot and if that is the case, then we can capture it in comments on
the lines:  (The check is skipped until replay reaches the remote
restart_lsn, as StandbyLogicalDecodingEnabledSince() describes
replayed WAL only and would otherwise reject a valid restart_lsn from
a lagging standby. Skipping it lets no bad slot through, as the slot
is not persisted in this cycle anyway. It starts out with the remote
restart_lsn, see reserve_wal_for_local_slot(), and
update_local_synced_slot() can neither find a serialized snapshot at
an LSN this standby has not replayed nor build one by decoding WAL it
has not replayed. It therefore reports no consistent snapshot, the
slot stays temporary, and a later cycle retries it once replay has
advanced.).

If the above reasoning is correct it doesn't seem like a good idea to
split the safety of the above mechanism in different functions.
Instead, we can move the new check just before
update_and_persist_local_synced_slot() and then avoid relying on the
code in update_and_persist_local_synced_slot() that can persist the
slot.

--
With Regards,
Amit Kapila.