Re: pg_upgrade and logical replication

On Mon, 19 Feb 2024 at 06:54, Hayato Kuroda (Fujitsu)
<kuroda.hayato@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