On Wed, Feb 12, 2025 at 12:57 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > Dear Shubham, > > Thanks for updating the patch! Few comments. > > 01. pg_createsubscriber.sgml > ``` > + <para> > + Remove all existing publications on the subscriber node before > creating > + a subscription. > + </para> > + > ``` >
Fixed. > I think this is not accurate. Publications on databases which are not target > will > retain. Also, I'm not sure it is important to clarify "before creating a > subscription.". > > How about: Remove all existing publications from specified databases. > > 02. main() > ``` > + opt.cleanup_existing_publications = false; > ``` > > ISTM initialization is done with the same ordering with > CreateSubscriberOptions. > Thus this should be at after recovery_timeout. > Fixed. > 03. 040_pg_createsubscriber.pl > ``` > +$node_p->wait_for_replay_catchup($node_s); > + > +# Verify the existing publications > +my $pub_count_before = > + $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"); > +is($pub_count_before, '2', > + 'two publications created before --cleanup-existing-publications is > run'); > ``` > > I think no need to declare $pub_count_before. How about: > > ``` > ok( $node_s->poll_query_until( > $db1, "SELECT COUNT(*) = 2 FROM pg_publication"), > 'two publications created before --cleanup-existing-publications is > run'); > ``` > Fixed. > 04. 040_pg_createsubscriber.pl > ``` +my $pub_count_after = > + $node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"); > +is($pub_count_after, '0', > + 'all publications dropped after --cleanup-existing-publications is > run'); > + > ``` > > I think no need to declare $pub_count_after. How about: > > ``` > is($node_s->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;"), '0', > 'all publications dropped after --cleanup-existing-publications is > run'); > ``` > Fixed. > 05. > > According to the document [1], we must do double-quote for each identifiers. > But current > patch seems not to do. Below example shows the case when three publications > exist. > > ``` > pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database > "postgres" > ``` > > I think the output must be `"aaa", "bbb", "ccc"`. This means > drop_publication() should be refactored. > > [1]: > https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES > Fixed. The attached patch at [1] contains the suggested changes. [1] - https://www.postgresql.org/message-id/CAHv8RjKGywu%2B3cAv9MPgxi1_TUXBT8yzUsW%2Bf%3Dg5UsgeJ8Uk6g%40mail.gmail.com Thanks and regards, Shubham Khanna.
