Re: pg_upgrade and logical replication

Thanks for addressing my past review comments.

Here are some more review comments for patch v16-0001

======
doc/src/sgml/ref/pgupgrade.sgml

1.
+      <para>
+       Create all the new tables that were created in the publication during
+       upgrade and refresh the publication by executing
+       <link linkend="sql-altersubscription"><command>ALTER
SUBSCRIPTION ... REFRESH PUBLICATION</command></link>.
+      </para>

"Create all ... that were created" sounds a bit strange.

SUGGESTION (maybe like this or similar?)
Create equivalent subscriber tables for anything that became newly
part of the publication during the upgrade and....

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

2. getSubscriptionTables

+/*
+ * getSubscriptionTables
+ *   Get information about subscription membership for dumpable tables. This
+ *    will be used only in binary-upgrade mode.
+ */
+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;

This function comment says "used only in binary-upgrade mode." and the
Assert says the same. But, is this compatible with the other function
dumpSubscriptionTable() where it says "used only in binary-upgrade
mode and for PG17 or later versions"?

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

3. check_new_cluster_subscription_configuration

+static void
+check_new_cluster_subscription_configuration(void)
+{
+ PGresult   *res;
+ PGconn    *conn;
+ int nsubs_on_old;
+ int max_replication_slots;
+
+ /* Logical slots can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return;

IMO it is better to say < 1700 in this check, instead of <= 1600.

~~~

4.
+ /* Quick return if there are no subscriptions to be migrated */
+ if (nsubs_on_old == 0)
+ return;

Missing period in comment.

~~~

5.
+/*
+ * check_old_cluster_subscription_state()
+ *
+ * Verify that each of the subscriptions has all their corresponding tables in
+ * i (initialize), r (ready) or s (synchronized) state.
+ */
+static void
+check_old_cluster_subscription_state(ClusterInfo *cluster)

This function is only for the old cluster (hint: the function name) so
there is no need to pass the 'cluster' parameter here. Just directly
use old_cluster in the function body.

======
src/bin/pg_upgrade/t/004_subscription.pl

6.
+# Add tab_not_upgraded1 to the publication. Now publication has tab_upgraded1
+# and tab_upgraded2 tables.
+$publisher->safe_psql('postgres',
+ "ALTER PUBLICATION regress_pub ADD TABLE tab_upgraded2");

Typo in comment. You added tab_not_upgraded2, not tab_not_upgraded1

~~

7.
+# Subscription relations should be preserved. The upgraded won't know
+# about 'tab_not_upgraded1' because the subscription is not yet refreshed.

Typo or missing word in comment?

"The upgraded" ??

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