changes_by_amit_1.patch.txt

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4a4bafba11..a4e723f922 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5014,18 +5014,21 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo)
 
 	appendPQExpBufferStr(query, ");\n");
 
+	/*
+	 * In binary-upgrade mode, we allow the replication to continue after the
+	 * upgrade.
+	 */
 	if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
 	{
 		if (subinfo->suboriginremotelsn)
 		{
 			/*
 			 * Preserve the remote_lsn for the subscriber's replication
-			 * origin. This value will be stale if the publisher gets
-			 * upgraded, we don't have a mechanism to distinguish this
-			 * scenario currently. There is no problem even if the remote_lsn
-			 * is updated with a stale value in this case as upgrade ensures
-			 * that all the transactions will be replicated before upgrading
-			 * the publisher.
+			 * origin. This value is required to start the replication from the
+			 * position before the upgrade. This value will be stale if the
+			 * publisher gets upgraded before the subscriber node. However,
+			 * this shouldn't be a problem as the upgrade ensures that all the
+			 * transactions were replicated before upgrading the publisher.
 			 */
 			appendPQExpBufferStr(query,
 								 "\n-- For binary upgrade, must preserve the remote_lsn for the subscriber's replication origin.\n");
@@ -5037,6 +5040,10 @@ dumpSubscription(Archive *fout, const SubscriptionInfo *subinfo)
 
 		if (strcmp(subinfo->subenabled, "t") == 0)
 		{
+			/*
+			 * Enable the subscription to allow the replication to continue
+			 * after the upgrade.
+			 */
 			appendPQExpBufferStr(query,
 								 "\n-- For binary upgrade, must preserve the subscriber's running state.\n");
 			appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s ENABLE;\n", qsubname);
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 4d6ae77e2d..9fd1417f0a 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -123,8 +123,8 @@ check_and_dump_old_cluster(bool live_check)
 		check_old_cluster_for_valid_slots(live_check);
 
 		/*
-		 * Subscription dependencies can be migrated since PG17. See comments
-		 * atop get_db_subscription_count().
+		 * Subscription and its dependencies can be migrated since PG17. See
+		 * comments atop get_db_subscription_count().
 		 */
 		check_old_cluster_subscription_state();
 	}
@@ -1554,7 +1554,8 @@ check_new_cluster_logical_replication_slots(void)
  * check_new_cluster_subscription_configuration()
  *
  * Verify that the max_replication_slots configuration specified is enough for
- * creating the subscriptions.
+ * creating the subscriptions. This is required to create the replication
+ * origin for each subscription.
  */
 static void
 check_new_cluster_subscription_configuration(void)
@@ -1564,7 +1565,7 @@ check_new_cluster_subscription_configuration(void)
 	int			nsubs_on_old;
 	int			max_replication_slots;
 
-	/* Logical slots can be migrated since PG17. */
+	/* Subscriptions and its dependencies can be migrated since PG17. */
 	if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700)
 		return;
 
@@ -1726,15 +1727,18 @@ check_old_cluster_subscription_state(void)
 		}
 
 		/*
-		 * A slot not created yet refers to the 'i' (initialize) state, while
-		 * 'r' (ready) state refers to a slot created previously but already
-		 * dropped. These states are supported for pg_upgrade. The other
+		 * We don't allow upgrade if there is a risk of dangling slot or origin
+		 * corresponding to initial sync after upgrade.
+		 *
+		 * A slot/origin not created yet refers to the 'i' (initialize) state,
+		 * while 'r' (ready) state refers to a slot/origin created previously but
+		 * already dropped. These states are supported for pg_upgrade. The other
 		 * states listed below are not supported:
 		 *
 		 * a) SUBREL_STATE_DATASYNC: A relation upgraded while in this state
 		 * would retain a replication slot, which could not be dropped by the
 		 * sync worker spawned after the upgrade because the subscription ID
-		 * tracked by the publisher does not match anymore.
+		 * used for the slot name won't match anymore.
 		 *
 		 * b) SUBREL_STATE_SYNCDONE: A relation upgraded while in this state
 		 * would retain the replication origin when there is a failure in
@@ -1786,6 +1790,7 @@ check_old_cluster_subscription_state(void)
 		fclose(script);
 		pg_log(PG_REPORT, "fatal");
 		pg_fatal("Your installation contains subscriptions without origin or having relations not in i (initialize) or r (ready) state.\n"
+				 "You can allow the initial sync to finish for all relations and then restart the upgrade.\n"
 				 "A list of the problem subscriptions is in the file:\n"
 				 "    %s", output_path);
 	}
diff --git a/src/bin/pg_upgrade/info.c b/src/bin/pg_upgrade/info.c
index fb8250002f..cc73c0fc0c 100644
--- a/src/bin/pg_upgrade/info.c
+++ b/src/bin/pg_upgrade/info.c
@@ -738,7 +738,7 @@ count_old_cluster_logical_slots(void)
 /*
  * get_db_subscription_count()
  *
- * Gets the number of subscription count of the database.
+ * Gets the number of subscriptions of the database referred to by "dbinfo".
  *
  * Note: This function will not do anything if the old cluster is pre-PG17.
  * This is because before that the logical slots are not upgraded, so we will