On Mon, 19 Feb 2024 at 06:54, Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> 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.
Thanks for the updated patch, I don't have any more comments.
Regards,
Vignesh
