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
