Re: Failure of subscription tests with topminnow

On Mon, Aug 30, 2021 at 5:48 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Mon, Aug 30, 2021 at 7:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> I have made the above changes on HEAD.
>

Thanks, this looks mostly good to me. I'll push and backpatch this
tomorrow unless you or someone else thinks otherwise.

Minor comments
==============
1.
$oldpid = $node_publisher->safe_psql('postgres',
- "SELECT pid FROM pg_stat_replication WHERE application_name = 'tap_sub';"
+ "SELECT pid FROM pg_stat_replication WHERE application_name =
'tap_sub' AND state = 'streaming';;"
);

An extra semicolon at the end of the statement.

2.
+# restart of subscription workers. We check the state along with
application_name
+# to ensure that the walsender is (re)started.

It is better to keep application_name in an above comment in the
second line as that will make this line looks a bit more consistent
with other comments.

3. In commit message, the text: "The reason was that the test was
assuming the walsender started before it reaches the 'streaming' state
and The check to test whether the subscription workers were restarting
after a change in the subscription was failing." seems to be
repeated/redundant.

4. Kindly submit the patches for back-branches.

--
With Regards,
Amit Kapila.