On Tue, Aug 31, 2021 at 3:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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,
>
Thanks Masahiko-san. I have included this change and made a new patch-set.
Hi Amit,
I have included your comments as well and also attached the patches
for the back-branches.
regards,
Ajin Cherian
Fujitsu Australia
