On Fri, 16 Feb 2024 at 08:22, Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> wrote:
>
> Dear Amit,
>
> > This sounds like a reasonable way to address the reported problem.
>
> OK, thanks!
>
> > Justin, do let me know if you think otherwise?
> >
> > Comment:
> > ===========
> > *
> > -# Setup an enabled subscription to verify that the running status and
> > failover
> > -# option are retained after the upgrade.
> > +# Setup a subscription to verify that the failover option are retained
> > after
> > +# the upgrade.
> > $publisher->safe_psql('postgres', "CREATE PUBLICATION regress_pub1");
> > $old_sub->safe_psql('postgres',
> > - "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$connstr' PUBLICATION
> > regress_pub1 WITH (failover = true)"
> > + "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$connstr'
> > PUBLICATION
> > regress_pub1 WITH (failover = true, enabled = false)"
> > );
> >
> > I think it is better not to create a subscription in the early stage
> > which we wanted to use for the success case. Let's have separate
> > subscriptions for failure and success cases. I think that will avoid
> > the newly added ALTER statements in the patch.
>
> I made a patch to avoid creating objects as much as possible, but it
> may lead some confusion. I recreated a patch for creating pub/sub
> and dropping them at cleanup for every test cases.
>
> PSA a 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.
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)"
);
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"
+);
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);
]);
Regards,
Vignesh
