On Fri, Aug 27, 2021 at 4:33 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Aug 27, 2021 at 3:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> >
> > I think the fix is correct but similar changes are required in
> > 022_twophase_cascade.pl as well (search for $oldpid in tests). I am
> > not completely sure but I think it is better to make this test change
> > in back branches as well to make it stable and reduce such random
> > build farm failures.
>
> I have made the changes in 022_twophase_cascade.pl for HEAD as well as
> patches for older branches.
>
Isn't it better to check the streaming state when we are fetching
oldpid? If we don't add, then I suspect that the next time someone
adding tests on similar lines might get confused about where to check
the state and where not. Also, if you agree, add some comments before
the test on why it is important to check states.
For ex., in below queries, the queries used for $oldpid.
my $oldpid = $node_publisher->safe_psql('postgres',
"SELECT pid FROM pg_stat_replication WHERE application_name =
'tap_sub';"
);
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr
sslmode=disable'"
);
$node_publisher->poll_query_until('postgres',
"SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = 'tap_sub' AND state = 'streaming';"
) or die "Timed out while waiting for apply to restart";
$oldpid = $node_publisher->safe_psql('postgres',
"SELECT pid FROM pg_stat_replication WHERE application_name =
'tap_sub';"
);
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only
WITH (copy_data = false)"
);
$node_publisher->poll_query_until('postgres',
"SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = 'tap_sub' AND state = 'streaming';"
--
With Regards,
Amit Kapila.
