On Thu, Sep 24, 2026 at 4:41 AM Nisha Moond <[email protected]> wrote: > > On Wed, Sep 23, 2026 at 1:45 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 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. > > > > Also, I think it's better to move the check to right after > > ReplicationSlotCreate() in synchronize_one_slot() 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. 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've attached the patch. > > > > 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. > > 2) The overview comment at the top of slotsync.c explains each reason > a slot isn't synced yet, but it doesn't mention this new > drop-and-retry case. Should we add it there too? The patch describes the details of the disabled/re-enabled case in synchronize_one_slot() and it looks sufficient to me. I've updated the patch for the above idea. In this version, the patch tracks only the last STATUS_CHANGE record that enables logical decoding, which makes it easy to check if logical decoding has continuously been enabled since the remote slot's restart_lsn. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
From adc7db5f52acc35115ea68969365a5ba64eff525 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada <[email protected]> Date: Tue, 22 Sep 2026 10:40:46 -0700 Subject: [PATCH v2] Fix slotsync when logical decoding is disabled and re-enabled. Commit 6aba42c660c made slot synchronization skip persisting a new slot if logical decoding got disabled after the remote slot information was fetched. As noted in its XXX comment, the check missed the case where the last logical slot on the primary is dropped and re-created with the same name in the meantime. If the standby had replayed both the deactivation and the re-activation by then, the slot was persisted with a restart_lsn preceding the deactivation. Subsequent synchronization cycles failed with "unexpected logical decoding status change" when advancing the slot, which also stopped the synchronization of all other failover slots, and the slot could not be decoded after promotion. Fix this by remembering the end LSN of the last replayed XLOG_LOGICAL_DECODING_STATUS_CHANGE record that enabled logical decoding, and checking the remote restart_lsn against it right after the local slot is created. The check runs only once the standby has replayed the remote restart_lsn, as nothing can be said about WAL it hasn't replayed yet. We compare the remote restart_lsn rather than the local one, so we may drop a slot that would have been usable, but the next synchronization cycle fetches fresh information. Reported-by: Nik Samokhvalov <[email protected]> Reviewed-by: shveta malik <[email protected]> Reviewed-by: Nisha Moond <[email protected]> Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=noi-onam_57pdh+f+...@mail.gmail.com Backpatch-through: 19 --- src/backend/access/transam/xlog.c | 2 +- src/backend/replication/logical/logicalctl.c | 65 ++++++++++++++- src/backend/replication/logical/slotsync.c | 82 +++++++++++-------- src/include/replication/logicalctl.h | 5 +- .../recovery/t/051_effective_wal_level.pl | 79 ++++++++++++++++++ 5 files changed, 194 insertions(+), 39 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 7b38c167eac..d8ac785b1f4 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -8782,7 +8782,7 @@ xlog_redo(XLogReaderState *record) * subsequent WAL records, which may not contain logical information. */ if (status) - EnableLogicalDecoding(); + EnableLogicalDecoding(lsn); else DisableLogicalDecoding(); diff --git a/src/backend/replication/logical/logicalctl.c b/src/backend/replication/logical/logicalctl.c index e5340880fa7..a2bef0aaa80 100644 --- a/src/backend/replication/logical/logicalctl.c +++ b/src/backend/replication/logical/logicalctl.c @@ -64,6 +64,7 @@ #include "postgres.h" #include "access/xloginsert.h" +#include "access/xlogrecovery.h" #include "catalog/pg_control.h" #include "miscadmin.h" #include "replication/slot.h" @@ -95,6 +96,22 @@ typedef struct LogicalDecodingCtlData /* True if logical decoding might need to be disabled */ bool pending_disable; + + /* + * End LSN of the last XLOG_LOGICAL_DECODING_STATUS_CHANGE record that + * enabled logical decoding, or InvalidXLogRecPtr if none has been + * replayed since the server started. Checking it with + * logical_decoding_enabled tells from which point on WAL was written with + * logical decoding enabled; see StandbyLogicalDecodingEnabledSince(). + * + * WAL records that disable logical decoding are deliberately not tracked + * here. logical_decoding_enabled is false while decoding is off, which is + * all the check needs. + * + * This is maintained only during recovery and is not persisted, so it + * says nothing about a status change replayed in an earlier server run. + */ + XLogRecPtr last_replayed_enable_lsn; } LogicalDecodingCtlData; static LogicalDecodingCtlData *LogicalDecodingCtl = NULL; @@ -210,6 +227,35 @@ IsLogicalDecodingEnabled(void) return enabled; } +/* + * Return true if logical decoding has been enabled continuously from the given + * LSN up to the current replay position, that is, if the WAL in that range + * was written with logical decoding enabled. + * + * The given LSN must have been replayed already; nothing can be said about + * WAL this server has not replayed yet. The caller is responsible for checking + * that. + * + * A true result therefore covers only the WAL replayed so far. A deactivation + * replayed after the call still applies, and callers must arrange for that + * themselves. + */ +bool +StandbyLogicalDecodingEnabledSince(XLogRecPtr lsn) +{ + bool result; + + Assert(RecoveryInProgress()); + Assert(lsn <= GetXLogReplayRecPtr(NULL)); + + LWLockAcquire(LogicalDecodingControlLock, LW_SHARED); + result = LogicalDecodingCtl->logical_decoding_enabled && + lsn >= LogicalDecodingCtl->last_replayed_enable_lsn; + LWLockRelease(LogicalDecodingControlLock); + + return result; +} + /* * Returns true if logical WAL logging is enabled based on the shared memory * status. @@ -330,21 +376,36 @@ EnsureLogicalDecodingEnabled(void) */ PG_ENSURE_ERROR_CLEANUP(abort_logical_decoding_activation, (Datum) 0); { - EnableLogicalDecoding(); + EnableLogicalDecoding(InvalidXLogRecPtr); } PG_END_ENSURE_ERROR_CLEANUP(abort_logical_decoding_activation, (Datum) 0); } /* * A workhorse function to enable logical decoding. + * + * lsn is the end LSN of the XLOG_LOGICAL_DECODING_STATUS_CHANGE record + * being replayed, and is InvalidXLogRecPtr when not called from redo. */ void -EnableLogicalDecoding(void) +EnableLogicalDecoding(XLogRecPtr lsn) { bool in_recovery; LWLockAcquire(LogicalDecodingControlLock, LW_EXCLUSIVE); + /* + * Remember where logical decoding was enabled. This has to happen before + * the early return below, because replay can reach here with the status + * already on. For example, CreateCheckPoint() fixes the redo point before + * it records logicalDecodingEnabled, so a checkpoint can claim logical + * decoding is enabled while the record that enabled it still follows the + * redo point. + */ + Assert(RecoveryInProgress() == XLogRecPtrIsValid(lsn)); + if (XLogRecPtrIsValid(lsn)) + LogicalDecodingCtl->last_replayed_enable_lsn = lsn; + /* Return if it is already enabled */ if (LogicalDecodingCtl->logical_decoding_enabled) { diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index c184cdf1c17..4c40278f573 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -709,41 +709,6 @@ update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid, return false; } - /* - * Do not persist the slot if logical decoding got disabled concurrently. - * This can happen if the last logical slot on the primary was dropped and - * the corresponding XLOG_LOGICAL_DECODING_STATUS_CHANGE record was - * replayed after we fetched the remote slot information: WAL records - * following the slot's restart_lsn might lack the information required by - * logical decoding, and the slot invalidation performed when replaying - * the record could not find our slot as it was not created yet. - * - * It is important to perform this check after creating the slot and - * before persisting it. This way, even if the status change record is - * replayed after this check, the replay will invalidate our slot. - * - * If the check fails, we keep the temporary slot and let the caller - * retry; the next cycle fetches the remote slot information again and - * will drop this slot as the remote slot no longer exists. - * - * 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. - */ - if (!IsLogicalDecodingEnabled()) - { - ereport(LOG, - errmsg("could not synchronize replication slot \"%s\"", - remote_slot->name), - errdetail("Logical decoding was concurrently disabled.")); - - if (slot_persistence_pending) - *slot_persistence_pending = true; - - return false; - } - ReplicationSlotPersist(); ereport(LOG, @@ -874,6 +839,7 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid, { NameData plugin_name; TransactionId xmin_horizon = InvalidTransactionId; + XLogRecPtr replay_lsn; /* Skip creating the local slot if remote_slot is invalidated already */ if (remote_slot->invalidated != RS_INVAL_NONE) @@ -892,6 +858,52 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid, remote_slot->failover, true); + /* + * 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 + * deactivation could not invalidate our slot because it did not exist + * yet, and WAL following the 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, so replay cannot get past that record behind our + * back. That is also why the status needs no recheck before the slot + * is persisted. (The invalidation is performed only in hot standby, + * which slot synchronization requires anyway.) + * + * WAL beyond the replay position tells us nothing, so a remote + * restart_lsn past it is accepted and left to the interlock above. + * + * 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; diff --git a/src/include/replication/logicalctl.h b/src/include/replication/logicalctl.h index 0bc1302f130..97cacb02e4e 100644 --- a/src/include/replication/logicalctl.h +++ b/src/include/replication/logicalctl.h @@ -14,14 +14,17 @@ #ifndef LOGICALCTL_H #define LOGICALCTL_H +#include "access/xlogdefs.h" + extern void StartupLogicalDecodingStatus(bool last_status); extern void InitializeProcessXLogLogicalInfo(void); extern bool ProcessBarrierUpdateXLogLogicalInfo(void); extern bool IsLogicalDecodingEnabled(void); +extern bool StandbyLogicalDecodingEnabledSince(XLogRecPtr lsn); extern bool IsXLogLogicalInfoEnabled(void); extern void AtEOXact_LogicalCtl(void); extern void EnsureLogicalDecodingEnabled(void); -extern void EnableLogicalDecoding(void); +extern void EnableLogicalDecoding(XLogRecPtr lsn); extern void RequestDisableLogicalDecoding(void); extern void DisableLogicalDecodingIfNecessary(void); extern void DisableLogicalDecoding(void); diff --git a/src/test/recovery/t/051_effective_wal_level.pl b/src/test/recovery/t/051_effective_wal_level.pl index 401a67ad1a6..c02cde6b2ce 100644 --- a/src/test/recovery/t/051_effective_wal_level.pl +++ b/src/test/recovery/t/051_effective_wal_level.pl @@ -597,6 +597,85 @@ select pg_sync_replication_slots(); '0', "no synced slot is left behind on standby5"); + # Test the same race, but where the slot is re-created on the primary + # before the slot synchronization resumes. Logical decoding is enabled + # again at the time the local slot is created, so checking the logical + # decoding status alone cannot tell that the remote slot information + # predates the deactivation. + + $primary->safe_psql('postgres', + qq[select pg_create_logical_replication_slot('sync_slot', 'test_decoding', false, false, true)] + ); + $primary->wait_for_replay_catchup($standby5); + test_wal_level($standby5, "replica|logical", + "logical decoding got activated on standby5 for the re-creation test" + ); + + $psql_sync_slot = $standby5->background_psql('postgres'); + $psql_sync_slot->query_until( + qr/sync_slots/, + q(\echo sync_slots +select injection_points_set_local(); +select injection_points_attach('replication-slot-create-begin', 'wait'); +select pg_sync_replication_slots(); +)); + $standby5->wait_for_event('client backend', + 'replication-slot-create-begin'); + + # Drop and re-create the slot, and wait for the standby to replay both + # the deactivation and the activation. + $primary->safe_psql('postgres', + qq[select pg_drop_replication_slot('sync_slot')]); + wait_for_logical_decoding_disabled($primary); + $primary->safe_psql('postgres', + qq[select pg_create_logical_replication_slot('sync_slot', 'test_decoding', false, false, true)] + ); + my $restart_lsn = $primary->safe_psql('postgres', + qq[select restart_lsn from pg_replication_slots where slot_name = 'sync_slot'] + ); + $primary->wait_for_replay_catchup($standby5); + test_wal_level($standby5, "replica|logical", + "logical decoding got deactivated and activated again on standby5"); + + # Resume the slot synchronization. It must drop the slot created from + # the stale information, and re-create it from the re-created remote slot + # on retry. + $log_offset = -s $standby5->logfile; + $standby5->safe_psql( + 'postgres', qq[ +select injection_points_detach('replication-slot-create-begin'); +select injection_points_wakeup('replication-slot-create-begin'); +]); + $standby5->wait_for_log( + qr/could not synchronize replication slot "sync_slot".*\n.*DETAIL: Logical decoding was disabled after the remote slot's restart LSN/, + $log_offset); + $standby5->poll_query_until('postgres', + qq[select restart_lsn >= '$restart_lsn' from pg_replication_slots where slot_name = 'sync_slot'] + ) + or die + "timed out waiting for the slot to be re-created from the re-created remote slot"; + + # The slot created on retry might not be persisted until the remote slot + # catches up with the catalog_xmin computed locally. Drop the remote slot + # to let the slot synchronization finish, keeping logical decoding enabled + # with another slot as the slot synchronization requires it. + $primary->safe_psql( + 'postgres', qq[ +select pg_create_logical_replication_slot('test_slot4', 'test_decoding'); +select pg_drop_replication_slot('sync_slot'); +]); + $primary->wait_for_replay_catchup($standby5); + $psql_sync_slot->quit; + $primary->safe_psql('postgres', + qq[select pg_drop_replication_slot('test_slot4')]); + wait_for_logical_decoding_disabled($primary); + $primary->wait_for_replay_catchup($standby5); + is( $standby5->safe_psql( + 'postgres', qq[select count(*) from pg_replication_slots]), + '0', + "no synced slot is left behind on standby5 after the re-creation test" + ); + # Test that logical slot creation on a standby fails cleanly if logical # decoding is concurrently deactivated by the end-of-recovery transition # upon promotion, which cannot find the slot that is not created yet. -- 2.55.0
