Re: pg_upgrade and logical replication

Here are some review comments for patch v17-0001

======
src/bin/pg_dump/pg_dump.c

1. getSubscriptionTables

+/*
+ * getSubscriptionTables
+ *   Get information about subscription membership for dumpable tables. This
+ *    will be used only in binary-upgrade mode and for PG17 or later versions.
+ */
+void
+getSubscriptionTables(Archive *fout)
+{
+ DumpOptions *dopt = fout->dopt;
+ SubscriptionInfo *subinfo = NULL;
+ SubRelInfo *subrinfo;
+ PQExpBuffer query;
+ PGresult   *res;
+ int i_srsubid;
+ int i_srrelid;
+ int i_srsubstate;
+ int i_srsublsn;
+ int ntups;
+ Oid last_srsubid = InvalidOid;
+
+ if (dopt->no_subscriptions || !dopt->binary_upgrade ||
+ fout->remoteVersion < 170000)
+ return;

I still felt that the function comment ("used only in binary-upgrade
mode and for PG17 or later") was misleading. IMO that sounds like it
would be OK for PG17 regardless of the binary mode, but the code says
otherwise.

Assuming the code is correct, perhaps the comment should say:
"... used only in binary-upgrade mode for PG17 or later versions."

~~~

2. dumpSubscriptionTable

+/*
+ * dumpSubscriptionTable
+ *   Dump the definition of the given subscription table mapping. This will be
+ *    used only in binary-upgrade mode and for PG17 or later versions.
+ */
+static void
+dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)

(this is the same as the previous review comment #1)

Assuming the code is correct, perhaps the comment should say:
"... used only in binary-upgrade mode for PG17 or later versions."

======
src/bin/pg_upgrade/check.c

3.
+static void
+check_old_cluster_subscription_state()
+{
+ FILE    *script = NULL;
+ char output_path[MAXPGPATH];
+ int ntup;
+ ClusterInfo *cluster = &old_cluster;
+
+ prep_status("Checking for subscription state");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "subs_invalid.txt");
+ for (int dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {
+ PGresult   *res;
+ DbInfo    *active_db = &cluster->dbarr.dbs[dbnum];
+ PGconn    *conn = connectToServer(cluster, active_db->db_name);

There seems no need for an extra variable ('cluster') here when you
can just reference 'old_cluster' directly in the code, the same as
other functions in this file do all the time.

======
Kind Regards,
Peter Smith.
Fujitsu Australia