Re: pg_upgrade and logical replication

Here are some review comments for patch v13-0001

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

1. getSubscriptionTables

+ int i_srsublsn;
+ int i;
+ int cur_rel = 0;
+ int ntups;

What is the difference between 'i' and 'cur_rel'?

AFAICT these represent the same tuple index, in which case you might
as well throw away 'cur_rel' and only keep 'i'.

~~~

2. getSubscriptionTables

+ for (i = 0; i < ntups; i++)
+ {
+ Oid cur_srsubid = atooid(PQgetvalue(res, i, i_srsubid));
+ Oid relid = atooid(PQgetvalue(res, i, i_srrelid));
+ TableInfo  *tblinfo;

Since this is all new code, using C99 style for loop variable
declaration of 'i' will be better.

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

3. check_for_subscription_state

+check_for_subscription_state(ClusterInfo *cluster)
+{
+ int dbnum;
+ FILE    *script = NULL;
+ char output_path[MAXPGPATH];
+ int ntup;
+
+ /* Subscription relations state can be migrated since PG17. */
+ if (GET_MAJOR_VERSION(old_cluster.major_version) < 1700)
+ return;
+
+ prep_status("Checking for subscription state");
+
+ snprintf(output_path, sizeof(output_path), "%s/%s",
+ log_opts.basedir,
+ "subscription_state.txt");

I felt this filename ought to be more like
'subscriptions_with_bad_state.txt' because the current name looks like
a normal logfile with nothing to indicate that it is only for the
states of the "bad" subscriptions.

~~~

4.
+ for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
+ {

Since this is all new code, using C99 style for loop variable
declaration of 'dbnum' will be better.

~~~

5.
+ * 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.

missing whitespace

/SUBREL_STATE_DATASYNC:A relation/SUBREL_STATE_DATASYNC: A relation/

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