On Sat, Feb 17, 2024 at 10:05 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 16 Feb 2024 at 10:50, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
>
> Thanks for the updated patch, Few suggestions:
> 1) This can be moved to keep it similar to other tests:
> +# Setup a disabled subscription. The upcoming test will check the
> +# pg_createsubscriber won't work, so it is sufficient.
> +$publisher->safe_psql('postgres', "CREATE PUBLICATION regress_pub1");
> +$old_sub->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$connstr'
> PUBLICATION regress_pub1 WITH (enabled = false)"
> +);
> +
> +$old_sub->stop;
> +
> +# ------------------------------------------------------
> +# Check that pg_upgrade fails when max_replication_slots configured in the new
> +# cluster is less than the number of subscriptions in the old cluster.
> +# ------------------------------------------------------
> +$new_sub->append_conf('postgresql.conf', "max_replication_slots = 0");
> +
> +# pg_upgrade will fail because the new cluster has insufficient
> +# max_replication_slots.
> +command_checks_all(
> + [
> + 'pg_upgrade', '--no-sync', '-d', $old_sub->data_dir,
> + '-D', $new_sub->data_dir, '-b', $oldbindir,
> + '-B', $newbindir, '-s', $new_sub->host,
> + '-p', $old_sub->port, '-P', $new_sub->port,
> + $mode, '--check',
> + ],
>
> like below and the extra comment can be removed:
> +# ------------------------------------------------------
> +# Check that pg_upgrade fails when max_replication_slots configured in the new
> +# cluster is less than the number of subscriptions in the old cluster.
> +# ------------------------------------------------------
> +# Create a disabled subscription.
>
It is okay to adjust as you are suggesting but I find Kuroda-San's
comment better than just saying: "Create a disabled subscription." as
that explicitly tells why it is okay to create a disabled
subscription.
>
> 3) The old comments were slightly better:
> # Resume the initial sync and wait until all tables of subscription
> # 'regress_sub5' are synchronized
> $new_sub->append_conf('postgresql.conf',
> "max_logical_replication_workers = 10");
> $new_sub->restart;
> $new_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub5 ENABLE");
> $new_sub->wait_for_subscription_sync($publisher, 'regress_sub5');
>
> Like:
> # Enable the subscription
> $new_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub5 ENABLE");
>
> # Wait until all tables of subscription 'regress_sub5' are synchronized
> $new_sub->wait_for_subscription_sync($publisher, 'regress_sub5');
>
I would prefer Kuroda-San's version as his version of the comment
explains the intent of the test better whereas what you are saying is
just exactly what the next line of code is doing and is
self-explanatory.
--
With Regards,
Amit Kapila.
