Re: pg_upgrade and logical replication

Here are some review comments for patch v20-0001

======

1. getSubscriptions

+ if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
+ appendPQExpBufferStr(query, " s.subenabled\n");
+ else
+ appendPQExpBufferStr(query, " false AS subenabled\n");

Probably I misunderstood this logic... AFAIK the CREATE SUBSCRIPTION
is normally default *enabled*, so why does this code set default
differently as 'false'. OTOH, if this is some special case default
needed because the subscription upgrade is not supported before PG17
then maybe it needs a comment to explain.

~~~

2. dumpSubscription

+ if (strcmp(subinfo->subenabled, "t") == 0)
+ {
+ appendPQExpBufferStr(query,
+ "\n-- For binary upgrade, must preserve the subscriber's running state.\n");
+ appendPQExpBuffer(query, "ALTER SUBSCRIPTION %s ENABLE;\n", qsubname);
+ }

(this is a bit similar to previous comment)

Probably I misunderstood this logic... but AFAIK the CREATE
SUBSCRIPTION is normally default *enabled*. In the CREATE SUBSCRIPTION
top of this function I did not see any "enabled=xxx" code, so won't
this just default to enabled=true per normal. In other words, what
happens if the subscription being upgraded was already DISABLED -- How
does it remain disabled still after upgrade?

But I saw there is a test case for this so perhaps the code is fine?
Maybe it just needs more explanatory comments for this area?

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

3.
+# The subscription's running status should be preserved
+my $result =
+  $new_sub->safe_psql('postgres',
+ "SELECT subenabled FROM pg_subscription WHERE subname = 'regress_sub'");
+is($result, qq(f),
+ "check that the subscriber that was disable on the old subscriber
should be disabled in the new subscriber"
+);
+$result =
+  $new_sub->safe_psql('postgres',
+ "SELECT subenabled FROM pg_subscription WHERE subname = 'regress_sub1'");
+is($result, qq(t),
+ "check that the subscriber that was enabled on the old subscriber
should be enabled in the new subscriber"
+);
+$new_sub->safe_psql('postgres', "DROP SUBSCRIPTION regress_sub1");
+

BEFORE
check that the subscriber that was disable on the old subscriber
should be disabled in the new subscriber

SUGGESTION
check that a subscriber that was disabled on the old subscriber is
disabled on the new subscriber

~

BEFORE
check that the subscriber that was enabled on the old subscriber
should be enabled in the new subscriber

SUGGESTION
check that a subscriber that was enabled on the old subscriber is
enabled on the new subscriber

~~~

4.
+is($result, qq($remote_lsn), "remote_lsn should have been preserved");
+
+
+# Check the number of rows for each table on each server


Double blank lines.

~~~

5.
+$old_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub1 DISABLE");
+$old_sub->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_sub1 SET (slot_name = none)");
+$old_sub->safe_psql('postgres', "DROP SUBSCRIPTION regress_sub1");
+

Probably it would be tidier to combine all of those.

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