On Thu, Nov 18, 2021 at 12:52 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > On Tuesday, November 16, 2021 2:31 PM Masahiko Sawada <sawada.m...@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' PUBLICATION tap_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? > > 2) > +...(streaming = on, two_phase = on);"); > Besides, is there some reasons to set two_phase to ? If so, > It might be better to add some comments about it. > Yes, two_phase = on is required by the tests for skip transaction patch. WIll remove it. > > 3) > +CREATE PUBLICATION tap_pub_streaming FOR TABLE test_tab_streaming; > +]); > + > > It seems there's no tests to use the table test_tab_streaming. I guess this > table is used to test streaming change error, maybe we can add some tests for > it ? Oops, similarly this is also required by the skip transaction tests. Will remove it. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
