On Tue, Aug 31, 2021 at 12:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 30, 2021 at 5:48 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Mon, Aug 30, 2021 at 7:52 PM Amit Kapila <amit.kapila16@gmail.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.
The patch with the above comments looks good to me. One minor
suggestion is to change the two messages of "die" to make the
investigation a bit easier. For example,
$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 after changing
CONNECTION";
and
$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 after changing
PUBLICATION";
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
