Re: Optionally automatically disable logical replication subscriptions on error

On Tue, Mar 8, 2022 at 6:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Mar 8, 2022 at 1:37 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> >
> > Kindly have a look at v30.
> >
>
> Review comments:
> ===============
>
Few comments on test script:
=======================
1.
+# This tests the uniqueness violation will cause the subscription
+# to fail during initial synchronization and make it disabled.

/This tests the/This tests that the

2.
+$node_publisher->safe_psql('postgres',
+ qq(CREATE PUBLICATION pub FOR TABLE tbl));
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+CREATE SUBSCRIPTION sub
+CONNECTION '$publisher_connstr'
+PUBLICATION pub WITH (disable_on_error = true)));

Please check other test scripts like t/015_stream.pl or
t/028_row_filter.pl and keep the indentation of these commands
similar. It looks odd and inconsistent with other tests. Also, we can
use double-quotes instead of qq so as to be consistent with other
scripts. Please check other similar places and make them consistent
with other test script files.

3.
+# Initial synchronization failure causes the subscription
+# to be disabled.

Here and in other places in test scripts, the comment lines seem too
short to me. Normally, we can keep it at the 80 char limit but this
appears too short.

4.
+# Delete the data from the subscriber and recreate the unique index.
+$node_subscriber->safe_psql(
+ 'postgres', q(
+DELETE FROM tbl;
+CREATE UNIQUE INDEX tbl_unique ON tbl (i)));

In other tests, we are executing single statements via safe_psql. I
don't see a problem with this but also don't see a reason to deviate
from the normal pattern.

--
With Regards,
Amit Kapila.