RE: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

Dear Shubham,

> I have reviewed and merged the proposed changes into the patch. The
> attached patches contain the suggested changes.

Thanks for updating the patch! Few comments:

01.
```
+       /*
+        * Fetch all databases from the source (publisher) if --all is specified.
+        */
+       if (opt.all_dbs)
+               fetch_source_databases(&opt);
+
        if (opt.database_names.head == NULL)
```

I feel we can convert "if" -> "else if" for the opt.database_names.head case,
because fetch_source_databases() ensures databases are listed.

02.
```
+# Set up node U as standby linking to node
```

To confirm: why can we use "U" here?

03.
```
+$node_u->append_conf(
+       'postgresql.conf', qq[
+primary_conninfo = '$pconnstr dbname=postgres'
+]);
```
I think this part is not required. init_from_backup() with has_streaming sets the
primary_conninfo.

04.
```
+# Stop $node_s
+$node_s->stop;
```

The comment does not describe anything. I feel you can say "Stop node S because
it won't be used anymore", or just remove it.

05.
```
+# Drop one of the databases (e.g., $db2)
+$node_p->safe_psql('postgres', "DROP DATABASE \"$db2\"");
```

"e.g." means "for example", so it is not suitable to put here. Please explain why it must be removed.

06.
```
+# Get subscription names
+$result = $node_u->safe_psql(
+       'postgres', qq(
+       SELECT subname FROM pg_subscription WHERE subname ~ '^pg_createsubscriber_'
+));
+my @subnames1 = split("\n", $result);
+
+# Wait subscriber to catch up
+$node_u->wait_for_subscription_sync($node_p, $subnames1[0]);
+$node_u->wait_for_subscription_sync($node_p, $subnames1[1]);
```

wait_for_subscription_sync() is used to wait until the initial synchronization is
done, so not suitable here. wait_for_catchup() is more appropriate.

07.
Also, the similar part exists on the pre-existing part (wait_for_subscription_sync
was used there, but I feel this may not be correct). How about unifing them like
attached?

08.
```
+# Verify logical replication works for all databases
+# Insert rows on node P
+$node_p->safe_psql('postgres', "INSERT INTO tbl1 VALUES('first row')");
```

This is confusing because 'fourth row' is inserted to $db1 just after it. How
about the 'row in database postgres' or something?

Best regards,
Hayato Kuroda
FUJITSU LIMITED