RE: Adding a '--clean-publisher-objects' option to 'pg_createsubscriber' utility. - Mailing list pgsql-hackers

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>
+
```

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.

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');
```

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');
```

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

Best regards,
Hayato Kuroda
FUJITSU LIMITED