Re: pg_upgrade and logical replication

Here are some review comments for patch v15-0001

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

1. getSubscriptions

+ if (fout->remoteVersion >= 170000)
+ appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n");
+ else
+ appendPQExpBufferStr(query, "NULL AS suboriginremotelsn\n");
+

There should be preceding spaces in those append strings to match the
other ones.

~~~

2. dumpSubscriptionTable

+/*
+ * dumpSubscriptionTable
+ * Dump the definition of the given subscription table mapping. This will be
+ * used only in binary-upgrade mode.
+ */
+static void
+dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
+{
+ DumpOptions *dopt = fout->dopt;
+ SubscriptionInfo *subinfo = subrinfo->subinfo;
+ PQExpBuffer query;
+ char *tag;
+
+ /* Do nothing in data-only dump */
+ if (dopt->dataOnly)
+ return;
+
+ Assert(fout->dopt->binary_upgrade || fout->remoteVersion >= 170000);

The function comment says this is only for binary-upgrade mode, so why
does the Assert use || (OR)?

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

3. check_and_dump_old_cluster

+ /*
+ * Subscription dependencies can be migrated since PG17. See comments atop
+ * get_old_cluster_subscription_count().
+ */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
+ check_old_cluster_subscription_state(&old_cluster);
+

Should this be combined with the other adjacent check so there is only
one "if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)"
needed?

~~~

4. check_new_cluster

check_new_cluster_logical_replication_slots();
+
+ check_new_cluster_subscription_configuration();

When checking the old cluster, the subscription was checked before the
slots, but here for the new cluster, the slots are checked before the
subscription. Maybe it makes no difference but it might be tidier to
do these old/new checks in the same order.

~~~

5. check_new_cluster_logical_replication_slots

- /* Quick return if there are no logical slots to be migrated. */
+ /* Quick return if there are no logical slots to be migrated */

Change is not relevant for this patch.

~~~

6.

+ res = executeQueryOrDie(conn, "SELECT setting FROM pg_settings "
+ "WHERE name IN ('max_replication_slots') "
+ "ORDER BY name DESC;");

Using IN and ORDER BY in this SQL seems unnecessary when you are only
searching for one name.

======
src/bin/pg_upgrade/info.c

7. statics

-
+static void get_old_cluster_subscription_count(DbInfo *dbinfo);

This change also removes an existing blank line -- not sure if that
was intentional

~~~

8.
@@ -365,7 +369,6 @@ get_template0_info(ClusterInfo *cluster)
PQfinish(conn);
}

-
/*
* get_db_infos()
*

This blank line change (before get_db_infos) should not be part of this patch.

~~~

9. get_old_cluster_subscription_count

It seems a slightly misleading function name because this is a PER-DB
count, not a cluster count.

~~~

10.
+ /* Subscriptions can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) <= 1600)
+ return;

IMO it is better to compare < 1700 instead of <= 1600. It keeps the
code more aligned with the comment.

~~~

11. count_old_cluster_subscriptions

+/*
+ * count_old_cluster_subscriptions()
+ *
+ * Returns the number of subscription for all databases.
+ *
+ * Note: this function always returns 0 if the old_cluster is PG16 and prior
+ * because we gather subscriptions only for cluster versions greater than or
+ * equal to PG17. See get_old_cluster_subscription_count().
+ */
+int
+count_old_cluster_subscriptions(void)
+{
+ int nsubs = 0;
+
+ for (int dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ nsubs += old_cluster.dbarr.dbs[dbnum].nsubs;
+
+ return nsubs;
+}

11a.
/subscription/subscriptions/

~

11b.
The code is now consistent with the slots code which looks good. OTOH
I thought that 'pg_catalog.pg_subscription' is shared across all
databases of the cluster, so isn't this code inefficient to be
querying again and again for every database (if there are many of
them) instead of just querying 1 time only for the whole cluster?

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

12.
It is difficult to keep track of all the tables (upgraded and not
upgraded) at each step of these tests. Maybe the comments can be more
explicit along the way. e.g

BEFORE
+# Add tab_not_upgraded1 to the publication

SUGGESTION
+# Add tab_not_upgraded1 to the publication. Now publication has <blah blah>

and

BEFORE
+# Subscription relations should be preserved

SUGGESTION
+# Subscription relations should be preserved. The upgraded won't know
about 'tab_not_upgraded1' because <blah blah>

etc.

~~~

13.
+$result =
+ $new_sub->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded1");
+is($result, qq(0),
+ "no change in table tab_not_upgraded1 afer enable subscription which
is not part of the publication"

/afer/after/

~~~

14.
+# ------------------------------------------------------
+# Check that pg_upgrade refuses to run a) if there's a subscription with tables
+# in a state different than 'r' (ready), 'i' (init) and 's' (synchronized)
+# and/or b) if the subscription does not have a replication origin.
+# ------------------------------------------------------

14a,
/does not have a/has no/

~

14b.
Maybe put a) and b) on newlines to be more readable.

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