On Mon, Mar 24, 2025 at 12:29 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@fujitsu.com> wrote: > > 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. >
Fixed.
> 02.
> ```
> +# Set up node U as standby linking to node
> ```
>
> To confirm: why can we use "U" here?
>
Since node_s and node_t are already used in this test, I have used the
next letter, node_u. Additionally, comments have been added to improve
clarity.
> 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.
>
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed.
> 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.
>
Fixed.
> 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?
>
Fixed.
> 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?
>
Fixed.
The attached patches contain the suggested changes.
Thanks and regards,
Shubham Khanna.
v19-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
v19-0002-Synopsis-for-all-option.patch
Description: Binary data
v19-0003-Additional-test-cases.patch
Description: Binary data
