On Wednesday, March 9, 2022 1:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Mar 8, 2022 at 6:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Tue, Mar 8, 2022 at 1:37 PM osumi.takamichi@fujitsu.com
> > <osumi.takamichi@fujitsu.com> wrote:
> > >
> > > Kindly have a look at v30.
> > >
> >
> > Review comments:
> > ===============
Thank you for reviewing !
> 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
Fixed.
> 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.
Fixed the inconsistent indentations within each commands.
Also, replace the qq with double-quotes (except for the is()'s
2nd argument, which is the aligned way to write the tests).
> 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.
Fixed.
> 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.
Fixed.
At the same time, I fixed one comment
where I should write "subscriber", not "sub",
since in the entire test file, I express the subscriber
by using the former.
The new patch v31 is shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373824855A6C4D2178027A0ED0A9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
