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. CreateSubscriberOptions
```
+    bool        cleanup_existing_publications;    /* drop all publications */
```

My previous comment [1] #1 did not intend to update attributes. My point was
only for the third argument of setup_subscriber().

02. setup_subscriber()
```
+            pg_log_info("cleaning up existing publications on the subscriber");
```

I don't think this output is needed. check_and_drop_existing_publications() has the similar output.

03. drop_publication_by_name()
```
+
+    appendPQExpBuffer(query, "DROP PUBLICATION %s", pubname_esc);
+    pg_log_info("dropping publication %s in database \"%s\"", pubname, dbname);
+    pg_log_debug("command is: %s", query->data);

```

Currently, appendPQExpBuffer() is done after the pg_log_info(). Please reserve current ordering if
there are no reasons. Also, variable "str" is currently used instead of query, please follow.

04. drop_publication_by_name()
```
    if (!dry_run)
     {
-        res = PQexec(conn, str->data);
+        res = PQexec(conn, query->data);
         if (PQresultStatus(res) != PGRES_COMMAND_OK)
+            dbinfo->made_publication = false;
+        else
         {
-            pg_log_error("could not drop publication \"%s\" in database \"%s\": %s",
-                         dbinfo->pubname, dbinfo->dbname, PQresultErrorMessage(res));
-            dbinfo->made_publication = false;    /* don't try again. */
-
-            /*
-             * Don't disconnect and exit here. This routine is used by primary
-             * (cleanup publication / replication slot due to an error) and
-             * subscriber (remove the replicated publications). In both cases,
-             * it can continue and provide instructions for the user to remove
-             * it later if cleanup fails.
-             */
+            pg_log_error("could not drop publication %s in database \"%s\": %s",
+                         pubname, dbname, PQresultErrorMessage(res));
         }
```

pg_log_error() is exected when the command succeed: This is not correct.

05. 040_pg_createsubscriber.pl
```
+# Set up node A as primary
+my $node_a = PostgreSQL::Test::Cluster->new('node_a');
+my $aconnstr = $node_a->connstr;
+$node_a->init(allows_streaming => 'logical');
+$node_a->append_conf('postgresql.conf', 'autovacuum = off');
+$node_a->start;
```

I don't think new primary is not needed. Can't you reuse node_p?

[1]:
https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED