Re: pg_upgrade and logical replication

On Thu, 30 Nov 2023 at 06:37, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.

No changes needed to be done in this case, explanation for the same is
given at [1]

> ~~~
>
> 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?

No changes needed to be done in this case, explanation for the same is
given at [1]

> ======
> 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

These statements are combined now

> ~~~
>
> 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.

Modified

> ~~~
>
> 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.

Modified

The changes for the same is present in the v21 version patch attached at [2]

[1] - https://www.postgresql.org/message-id/CAA4eK1JpWkRBFMDC3wOCK%3DHzCXg8XT1jH-tWb%3Db%2B%2B_8YS2%3DQSQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CALDaNm37E4tmSZd%2Bk1ixtKevX3eucmhdOnw4pGmykZk4C1Nm4Q%40mail.gmail.com

Regards,
Vignesh