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
kuroda.diffs
Description: kuroda.diffs
