On Fri, Nov 13, 2026 at 7:35 PM Amit Kapila <[email protected]> wrote:
>
> On Fri, Nov 13, 2026 at 7:29 PM Bertrand Drouvot
> <[email protected]> wrote:
> >
> > On Fri, Nov 13, 2026 at 04:45:00PM +0530, Amit Kapila wrote:
> > >
> > > + else if (slot->data.persistency == RS_TEMPORARY)
> > > + {
> > > + /*
> > > + * We reach this point when a previous cycle updated confirmed_flush
> > > + * but failed to build a consistent snapshot, and the remote slot has
> > > + * not advanced beyond the local slot since then.
> > > + */
> > > + skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
> > > + }
> > >
> > > Is it better to directly use the slot's skip_reason rather than
> > > assuming SS_SKIP_NO_CONSISTENT_SNAPSHOT?
> >
> > That could work but what about the case I mentioned upthread where the local
> > catalog_xmin gets ahead and the remote later catches up? I think that that
> > would
> > keep slotsync_skip_reason as SS_SKIP_WAL_OR_ROWS_REMOVED, although the slot
> > is
> > still inconsistent due to the missing snapshot.
> >
>
> Right the reason can remain as SS_SKIP_WAL_OR_ROWS_REMOVED. Even in
> the first cycle, the reason could be either SS_SKIP_WAL_NOT_FLUSHED or
> SS_SKIP_WAL_OR_ROWS_REMOVED and in that case also the caller
> update_and_persist_local_synced_slot() won't persist the slot. So, I
> don't see a problem with retaining the previous reason when we didn't
> perform anything in a particular sync cycle.
While I agree that retaining the previous reason doesn't make us
persist the slot, I think it's safe only because the previous reason
is never SS_SKIP_NONE for a temporary slot at that point. That's true
today, but it depends on several things and it would be hard to modify
these code correctly in the future.
I think the root cause is that commit 788ec96d591 replaced
found_consistent_snapshot with the check of slotsync_skip_reason, but
they're not equivalent: found_consistent_snapshot is false unless the
current cycle synced the slot and found a consistent snapshot, whereas
slotsync_skip_reason is SS_SKIP_NONE unless the current cycle detected
a reason to skip. So an alternative idea would be to bring back
found_consistent_snapshot part from the commit. That way, the decision
of persisting the slot doesn't depend on the skip_reasons at all.
As for the reported reason, a temporary slot is the one we have not
confirmed to reach a consistent point yet. So I think we can simply
report no_consistent_snapshot for a temporary slot if the current
cycle didn't find a consistent snapshot, without relying on what
happened in the previous cycles.
I've attached a PoC patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index aa4cc436ad7..1e760a798e4 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -217,18 +217,27 @@ update_slotsync_skip_stats(SlotSyncSkipReason skip_reason)
*
* If no update was needed (the data of the remote slot is the same as the
* local slot) return false, otherwise true.
+ *
+ * *found_consistent_snapshot will be true iff the remote slot's LSN or xmin is
+ * synced in this call, and decoding from the corresponding LSN's can reach a
+ * consistent snapshot.
*/
static bool
-update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
+update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
+ bool *found_consistent_snapshot)
{
ReplicationSlot *slot = MyReplicationSlot;
bool updated_xmin_or_lsn = false;
bool updated_config = false;
+ bool found_consistent = false;
SlotSyncSkipReason skip_reason = SS_SKIP_NONE;
XLogRecPtr latestFlushPtr = GetStandbyFlushRecPtr(NULL);
Assert(slot->data.invalidated == RS_INVAL_NONE);
+ if (found_consistent_snapshot)
+ *found_consistent_snapshot = false;
+
/*
* Make sure that concerned WAL is received and flushed before syncing
* slot to target lsn received from the primary server.
@@ -335,16 +344,16 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
SpinLockRelease(&slot->mutex);
updated_xmin_or_lsn = true;
+ found_consistent = true;
}
else
{
- bool found_consistent_snapshot;
XLogRecPtr old_confirmed_lsn = slot->data.confirmed_flush;
XLogRecPtr old_restart_lsn = slot->data.restart_lsn;
TransactionId old_catalog_xmin = slot->data.catalog_xmin;
LogicalSlotAdvanceAndCheckSnapState(remote_slot->confirmed_lsn,
- &found_consistent_snapshot);
+ &found_consistent);
/* Sanity check */
if (slot->data.confirmed_flush != remote_slot->confirmed_lsn)
@@ -359,7 +368,7 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
* If we can't reach a consistent snapshot, the slot won't be
* persisted. See update_and_persist_local_synced_slot().
*/
- if (!found_consistent_snapshot)
+ if (!found_consistent)
{
Assert(MyReplicationSlot->data.persistency == RS_TEMPORARY);
@@ -383,6 +392,18 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid)
}
}
+ /*
+ * A temporary slot is not sync-ready until we find that decoding from its
+ * restart_lsn reaches a consistent point. That's the case not only when
+ * we failed to find one above, but also when the remote slot has not
+ * advanced beyond the local slot and so we didn't attempt to sync it.
+ */
+ if (slot->data.persistency == RS_TEMPORARY && !found_consistent)
+ skip_reason = SS_SKIP_NO_CONSISTENT_SNAPSHOT;
+
+ if (found_consistent_snapshot)
+ *found_consistent_snapshot = found_consistent;
+
/* Update slot sync skip stats */
update_slotsync_skip_stats(skip_reason);
@@ -678,21 +699,25 @@ update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
bool *slot_persistence_pending)
{
ReplicationSlot *slot = MyReplicationSlot;
+ bool found_consistent_snapshot;
/* Slotsync skip stats are handled in function update_local_synced_slot() */
- (void) update_local_synced_slot(remote_slot, remote_dbid);
+ (void) update_local_synced_slot(remote_slot, remote_dbid,
+ &found_consistent_snapshot);
/*
- * Check if the slot cannot be synchronized. Refer to the comment atop the
- * file for details on this check.
+ * Persist the slot only if this cycle has synced it to the remote slot
+ * and found that decoding from its restart_lsn reaches a consistent
+ * point. Refer to the comment atop the file for details on this check.
*/
- if (slot->slotsync_skip_reason != SS_SKIP_NONE)
+ if (!found_consistent_snapshot)
{
/*
* We reach this point when the remote slot didn't catch up to locally
* reserved position, or it cannot reach the consistent point from the
* restart_lsn, or the WAL prior to the remote confirmed flush LSN has
- * not been received and flushed.
+ * not been received and flushed, or there was nothing to sync because
+ * the remote slot has not advanced beyond the local slot.
*
* We do not drop the slot because the restart_lsn and confirmed_lsn
* can be ahead of the current location when recreating the slot in
@@ -709,6 +734,8 @@ update_and_persist_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
return false;
}
+ Assert(slot->slotsync_skip_reason == SS_SKIP_NONE);
+
ReplicationSlotPersist();
ereport(LOG,
@@ -831,7 +858,8 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid,
LSN_FORMAT_ARGS(slot->data.confirmed_flush),
LSN_FORMAT_ARGS(remote_slot->confirmed_lsn)));
- slot_updated = update_local_synced_slot(remote_slot, remote_dbid);
+ slot_updated = update_local_synced_slot(remote_slot, remote_dbid,
+ NULL);
}
}
/* Otherwise create the slot first. */
diff --git a/src/test/recovery/t/040_standby_failover_slots_sync.pl b/src/test/recovery/t/040_standby_failover_slots_sync.pl
index b0f4dfad6f2..8272dae95c7 100644
--- a/src/test/recovery/t/040_standby_failover_slots_sync.pl
+++ b/src/test/recovery/t/040_standby_failover_slots_sync.pl
@@ -1114,4 +1114,57 @@ $standby2->wait_for_log(
$h->quit;
+##################################################
+# Test that a synced slot stays temporary when the standby cannot build a
+# consistent snapshot across multiple sync cycles.
+##################################################
+
+# Disable the subscription so lsub1_slot does not advance and serialize
+# snapshots on standby2 while still holding back catalog_xmin on standby2.
+$subscriber1->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub1 DISABLE");
+$primary->poll_query_until('postgres',
+ "SELECT NOT active FROM pg_replication_slots WHERE slot_name = 'lsub1_slot'"
+);
+
+$primary->safe_psql('postgres',
+ "SELECT pg_create_logical_replication_slot('inconsistent_slot', 'test_decoding', false, false, true);"
+);
+
+# Keep a transaction with an assigned XID open while logging xl_running_xacts
+# and advancing inconsistent_slot so its restart_lsn moves to that record and
+# concurrent bgwriter snapshots cannot make it consistent. Advance the slot
+# twice in case a concurrent xl_running_xacts record logged before the open
+# transaction was used as the candidate restart_lsn in the first call.
+my $xact_h = $primary->background_psql('postgres');
+$xact_h->query_safe('BEGIN; SELECT pg_current_xact_id();');
+
+$primary->safe_psql(
+ 'postgres', qq(
+ SELECT pg_log_standby_snapshot();
+ BEGIN;
+ INSERT INTO tab_int VALUES (100);
+ COMMIT;
+ SELECT pg_replication_slot_advance('inconsistent_slot', pg_current_wal_lsn());
+ SELECT pg_replication_slot_advance('inconsistent_slot', pg_current_wal_lsn());
+));
+
+$xact_h->query_safe('COMMIT;');
+$xact_h->quit;
+
+$primary->wait_for_replay_catchup($standby2);
+
+$standby2->append_conf('postgresql.conf', 'sync_replication_slots = on');
+$standby2->reload;
+
+ok( $standby2->poll_query_until(
+ 'postgres', qq(
+ SELECT slotsync_skip_count >= 2 AND temporary AND
+ slotsync_skip_reason = 'no_consistent_snapshot'
+ FROM pg_stat_replication_slots
+ JOIN pg_replication_slots USING (slot_name)
+ WHERE slot_name = 'inconsistent_slot';
+ )),
+ 'synced slot without a consistent snapshot stays temporary');
+
done_testing();
