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
