From 2e8b87416042b2612b8bb930b4dbc6c7cb8c8155 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada Date: Tue, 22 Sep 2026 10:40:46 -0700 Subject: [PATCH v1] 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, and by requiring the remote restart_lsn to be at or after it right after creating the local slot. Once the slot exists, a later deactivation invalidates it, so remove the check before persisting the slot. We compare the remote restart_lsn rather than the local one, so we may drop a slot that would have been usable, but the retry in the next synchronization cycle fetches fresh information. Reported-by: Nik Samokhvalov Reviewed-by: Discussion: https://postgr.es/m/CAM527d_eV_BAYFiQnfZLSPfHoihye=nOi-OnAM_57pdH+F+gfA@mail.gmail.com Backpatch-through: 19 --- src/backend/access/transam/xlog.c | 1 + src/backend/replication/logical/logicalctl.c | 50 +++++++++++ src/backend/replication/logical/slotsync.c | 83 +++++++++++-------- src/include/replication/logicalctl.h | 4 + .../recovery/t/051_effective_wal_level.pl | 79 ++++++++++++++++++ 5 files changed, 182 insertions(+), 35 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 9ec0be77ca0..894cb83915d 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -9648,6 +9648,7 @@ xlog_redo(XLogReaderState *record) * disabling logical decoding because logical decoding cannot process * subsequent WAL records, which may not contain logical information. */ + SetLogicalDecodingStatusChangeLSN(record->EndRecPtr); if (status) EnableLogicalDecoding(); else diff --git a/src/backend/replication/logical/logicalctl.c b/src/backend/replication/logical/logicalctl.c index 642d965bd1c..d540763220c 100644 --- a/src/backend/replication/logical/logicalctl.c +++ b/src/backend/replication/logical/logicalctl.c @@ -95,6 +95,13 @@ 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 + * replayed, or InvalidXLogRecPtr if none has been replayed since the + * server started. Only maintained during recovery. + */ + XLogRecPtr status_change_end_lsn; } LogicalDecodingCtlData; static LogicalDecodingCtlData *LogicalDecodingCtl = NULL; @@ -210,6 +217,49 @@ IsLogicalDecodingEnabled(void) return enabled; } +/* + * Return true if logical decoding is enabled and the given lsn is at or after + * the end of the XLOG_LOGICAL_DECODING_STATUS_CHANGE record that enabled it + * last. This means that WAL from the lsn onward was written with logical + * decoding enabled. + * + * Note that status_change_end_lsn lives only in shared memory and is updated + * only by WAL replay, so this is accurate only during recovery and for an lsn + * at or after the point where WAL replay started in this server run. For an + * older lsn, this returns the same result as IsLogicalDecodingEnabled() if no + * status change record has been replayed yet, and false otherwise. + */ +bool +IsLogicalDecodingEnabledSince(XLogRecPtr lsn) +{ + bool result; + + LWLockAcquire(LogicalDecodingControlLock, LW_SHARED); + result = LogicalDecodingCtl->logical_decoding_enabled && + lsn >= LogicalDecodingCtl->status_change_end_lsn; + LWLockRelease(LogicalDecodingControlLock); + + return result; +} + +/* + * Remember the end LSN of the XLOG_LOGICAL_DECODING_STATUS_CHANGE record + * being replayed. + * + * This must be called before the record changes the status, so that + * IsLogicalDecodingEnabledSince() never sees the new status paired with the + * LSN of an older record. + */ +void +SetLogicalDecodingStatusChangeLSN(XLogRecPtr lsn) +{ + Assert(RecoveryInProgress()); + + LWLockAcquire(LogicalDecodingControlLock, LW_EXCLUSIVE); + LogicalDecodingCtl->status_change_end_lsn = lsn; + LWLockRelease(LogicalDecodingControlLock); +} + /* * Returns true if logical WAL logging is enabled based on the shared memory * status. diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index c0403893e23..b12d3e0f458 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -718,41 +718,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, @@ -901,6 +866,54 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid, remote_slot->failover, true); + /* + * The remote slot information might predate an + * XLOG_LOGICAL_DECODING_STATUS_CHANGE record that was replayed before + * our slot was created. This happens if the last logical slot on the + * primary was dropped, and possibly re-created with the same name, + * after we fetched the remote slot information. The slot invalidation + * performed when replaying the record could not find our slot, and + * WAL records following the remote restart_lsn might lack the + * information required by logical decoding. Checking only whether + * logical decoding is enabled is not enough since it might have been + * disabled and enabled again in the meantime. + * + * IsLogicalDecodingEnabledSince() knows only about the status changes + * replayed in this server run; if none has been replayed, it gives + * the same result as IsLogicalDecodingEnabled(). That's enough here + * because what we need to detect is a status change written by the + * primary after we fetched the remote slot information, which is + * always replayed in this server run. Status changes written before + * the fetch don't matter, as the primary writes a deactivation record + * only when it has no valid logical slot: the remote restart_lsn is + * always past such a record and the following activation record. + * + * It is important to perform this check after creating the slot. If a + * status change record is replayed after this check, the replay + * invalidates our slot. So once a slot passes this check, its + * restart_lsn stays decodable until it is invalidated, and we don't + * need to check it again before persisting the slot. + * + * If the check fails, drop the slot and let the caller retry; the + * next cycle fetches the remote slot information again. We cannot + * keep the slot as it would continue to use the stale restart_lsn. + */ + if (!IsLogicalDecodingEnabledSince(remote_slot->restart_lsn)) + { + 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))); + + 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..73333e133e2 100644 --- a/src/include/replication/logicalctl.h +++ b/src/include/replication/logicalctl.h @@ -14,10 +14,14 @@ #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 IsLogicalDecodingEnabledSince(XLogRecPtr lsn); +extern void SetLogicalDecodingStatusChangeLSN(XLogRecPtr lsn); extern bool IsXLogLogicalInfoEnabled(void); extern void AtEOXact_LogicalCtl(void); extern void EnsureLogicalDecodingEnabled(void); diff --git a/src/test/recovery/t/051_effective_wal_level.pl b/src/test/recovery/t/051_effective_wal_level.pl index b11690863d9..6ca7e273542 100644 --- a/src/test/recovery/t/051_effective_wal_level.pl +++ b/src/test/recovery/t/051_effective_wal_level.pl @@ -598,6 +598,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 on the standby has not been continuously enabled since 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. -- 2.55.0