RE: pg_upgrade and logical replication

Dear Vignesh,

Thanks for reviewing! PSA new version.

> 
> 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.
> +$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;
> +
> +$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',
> +       ],

Partially fixed. I moved the creation part to below but comments were kept.

> 2) This comment can be slightly changed:
> +# Change configuration as well not to start the initial sync automatically
> +$new_sub->append_conf('postgresql.conf',
> +       "max_logical_replication_workers = 0");
> 
> to:
> Change configuration so that initial table sync sync does not get
> started automatically

Fixed.

> 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');

Per comments from Amit [1], I did not change.

[1]: https://www.postgresql.org/message-id/CAA4eK1Ls%2BRmJtTvOgaRXd%2BeHSY3x-KUE%3DsfEGQoU-JF_UzA62A%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/