Re: Skipping logical replication transactions on subscriber side

On Thu, Nov 18, 2021 at 12:52 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, November 16, 2021 2:31 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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/