v1-0001-Correct-sanity-check-to-compare-confirmed_lsn.patch

From 24d3c8b5db1e75bdea37528d834aa9f62c634a9c Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.malik@gmail.com>
Date: Thu, 4 Apr 2024 14:18:51 +0530
Subject: [PATCH v1] Correct sanity check to compare confirmed_lsn.

Corrected sanity check to compare confirmed_lsn rather than
restart_lsn to handle BF failure.

Other changes are:
A log has been added after LogicalSlotAdvanceAndCheckSnapState()
to log the case when the local and remote slots' confirmed-lsn
were not found to be the same after sync.

Now we attempt sync in update_local_synced_slot() if one of confirmed_lsn,
restart_lsn, and catalog_xmin for remote slot is ahead of local slot instead
of them just being inequal.
---
 src/backend/replication/logical/slotsync.c | 25 ++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c
index 9ac847b780..9d022a78af 100644
--- a/src/backend/replication/logical/slotsync.c
+++ b/src/backend/replication/logical/slotsync.c
@@ -174,9 +174,14 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
 	if (found_consistent_snapshot)
 		*found_consistent_snapshot = false;
 
-	if (remote_slot->confirmed_lsn != slot->data.confirmed_flush ||
-		remote_slot->restart_lsn != slot->data.restart_lsn ||
-		remote_slot->catalog_xmin != slot->data.catalog_xmin)
+	/*
+	 * Attempt to sync lsns and xmins only if remote slot is ahead of local
+	 * slot.
+	 */
+	if (remote_slot->confirmed_lsn > slot->data.confirmed_flush ||
+		remote_slot->restart_lsn > slot->data.restart_lsn ||
+		TransactionIdFollows(remote_slot->catalog_xmin,
+							 slot->data.catalog_xmin))
 	{
 		/*
 		 * We can't directly copy the remote slot's LSN or xmin unless there
@@ -207,6 +212,14 @@ update_local_synced_slot(RemoteSlot *remote_slot, Oid remote_dbid,
 		{
 			LogicalSlotAdvanceAndCheckSnapState(remote_slot->confirmed_lsn,
 												found_consistent_snapshot);
+
+			if (slot->data.confirmed_flush != remote_slot->confirmed_lsn)
+				elog(LOG,
+					 "could not synchronize local slot \"%s\" LSN(%X/%X)"
+					 " to remote slot's LSN(%X/%X) ",
+					 remote_slot->name,
+					 LSN_FORMAT_ARGS(slot->data.confirmed_flush),
+					 LSN_FORMAT_ARGS(remote_slot->confirmed_lsn));
 		}
 
 		ReplicationSlotsComputeRequiredXmin(false);
@@ -628,13 +641,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid remote_dbid)
 			 * Sanity check: As long as the invalidations are handled
 			 * appropriately as above, this should never happen.
 			 */
-			if (remote_slot->restart_lsn < slot->data.restart_lsn)
+			if (remote_slot->confirmed_lsn < slot->data.confirmed_flush)
 				elog(ERROR,
 					 "cannot synchronize local slot \"%s\" LSN(%X/%X)"
 					 " to remote slot's LSN(%X/%X) as synchronization"
 					 " would move it backwards", remote_slot->name,
-					 LSN_FORMAT_ARGS(slot->data.restart_lsn),
-					 LSN_FORMAT_ARGS(remote_slot->restart_lsn));
+					 LSN_FORMAT_ARGS(slot->data.confirmed_flush),
+					 LSN_FORMAT_ARGS(remote_slot->confirmed_lsn));
 
 			/* Make sure the slot changes persist across server restart */
 			if (update_local_synced_slot(remote_slot, remote_dbid, NULL))
-- 
2.34.1