Re: pg_upgrade and logical replication - Mailing list pgsql-hackers

On Fri, 16 Feb 2024 at 10:50, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! PSA new version.
>
> >
> > Thanks for the updated patch, few suggestions:
> > 1) Can we use a new publication for this subscription too so that the
> > publication and subscription naming will become consistent throughout
> > the test case:
> > +# Table will be in 'd' (data is being copied) state as table sync will fail
> > +# because of primary key constraint error.
> > +my $started_query =
> > +  "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'd'";
> > +$old_sub->poll_query_until('postgres', $started_query)
> > +  or die
> > +  "Timed out while waiting for the table state to become 'd' (datasync)";
> > +
> > +# Create another subscription and drop the subscription's replication origin
> > +$old_sub->safe_psql('postgres',
> > +       "CREATE SUBSCRIPTION regress_sub3 CONNECTION '$connstr'
> > PUBLICATION regress_pub2 WITH (enabled = false)"
> > +);
> >
> > So after the change it will become like subscription regress_sub3 for
> > publication regress_pub3, subscription regress_sub4 for publication
> > regress_pub4 and subscription regress_sub5 for publication
> > regress_pub5.
>
> A new publication was defined.
>
> > 2) The tab_upgraded1 table can be created along with create
> > publication and create subscription itself:
> > $publisher->safe_psql('postgres',
> > "CREATE PUBLICATION regress_pub3 FOR TABLE tab_upgraded1");
> > $old_sub->safe_psql('postgres',
> > "CREATE SUBSCRIPTION regress_sub4 CONNECTION '$connstr' PUBLICATION
> > regress_pub3 WITH (failover = true)"
> > );
>
> The definition of tab_upgraded1 was moved to the place you pointed.
>
> > 3) The tab_upgraded2 table can be created along with create
> > publication and create subscription itself to keep it consistent:
> >  $publisher->safe_psql('postgres',
> > -       "ALTER PUBLICATION regress_pub2 ADD TABLE tab_upgraded2");
> > +       "CREATE PUBLICATION regress_pub4 FOR TABLE tab_upgraded2");
> >  $old_sub->safe_psql('postgres',
> > -       "ALTER SUBSCRIPTION regress_sub2 REFRESH PUBLICATION");
> > +       "CREATE SUBSCRIPTION regress_sub5 CONNECTION '$connstr'
> > PUBLICATION regress_pub4"
> > +);
>
> Ditto.
>
> > With above fixes, the following can be removed:
> > # Initial setup
> > $publisher->safe_psql(
> > 'postgres', qq[
> > CREATE TABLE tab_upgraded1(id int);
> > CREATE TABLE tab_upgraded2(id int);
> > ]);
> > $old_sub->safe_psql(
> > 'postgres', qq[
> > CREATE TABLE tab_upgraded1(id int);
> > CREATE TABLE tab_upgraded2(id int);
> > ]);
>
> Yes, earlier definitions were removed instead.
> Also, some comments were adjusted based on these fixes.

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

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

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

Regards,
Vignesh