Re: Skipping logical replication transactions on subscriber side

On Wed, Nov 24, 2021 at 1:50 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Nov 24, 2021 at 12:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Nov 24, 2021 at 7:51 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, Nov 18, 2021 at 12:52 PM houzj.fnst@fujitsu.com
> > > <houzj.fnst@fujitsu.com> wrote:
> > > >
> > > > On Tuesday, November 16, 2021 2:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > > > Right. I've fixed this issue and attached an updated patch.
> > > > >
> > > > Hi,
> > > >
> > > > I have few comments for the testcases.
> > > >
> > > > 1)
> > > >
> > > > +my $appname = 'tap_sub';
> > > > +$node_subscriber->safe_psql(
> > > > +    'postgres',
> > > > +    "CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr application_name=$appname' PUBLICATION tap_pub
WITH(streaming = off, two_phase = on);");
 
> > > > +my $appname_streaming = 'tap_sub_streaming';
> > > > +$node_subscriber->safe_psql(
> > > > +    'postgres',
> > > > +    "CREATE SUBSCRIPTION tap_sub_streaming CONNECTION '$publisher_connstr application_name=$appname_streaming'
PUBLICATIONtap_pub_streaming WITH (streaming = on, two_phase = on);");
 
> > > > +
> > > >
> > > > I think we can remove the 'application_name=$appname', so that the command
> > > > could be shorter.
> > >
> > > But we wait for the subscription to catch up by using
> > > wait_for_catchup() with application_name, no?
> > >
> >
> > Yeah, but you can directly use the subscription name in
> > wait_for_catchup because we internally use that as
> > fallback_application_name. If application_name is not specified in the
> > connection string as suggested by Hou-San then
> > fallback_application_name will be considered. Both ways are okay and I
> > see we use both ways in the tests but it seems there are more places
> > where we use the method Hou-San is suggesting in subscription tests.
>
> Okay, thanks! I referred to tests that set application_name. ISTM it's
> better to unite them so as not to confuse them in future tests.
>

Agreed, but let's do this clean-up as a separate patch. Feel free to
submit the patch for the same in a separate thread.

-- 
With Regards,
Amit Kapila.