On Wed, Feb 12, 2025 at 12:57 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@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.
