On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch!
>
> > I have added a test case for non-dry-run mode to ensure that
> > replication slots, subscriptions, and publications work as expected
> > when '--all' is specified. Additionally, I have split the test file
> > into two parts:
> > 1) Command-line sanity checks – Validates the correctness of option parsing.
> > 2) '--all' functionality and behavior – Verifies the health of
> > subscriptions and ensures that --all specific scenarios, such as
> > non-template databases not being subscribed to, are properly handled.
> > This should improve test coverage while keeping the structure manageable.
>
> TBH, I feel your change does not separate the test file into the two parts. ISTM
> you just added some validation checks and a test how --all option works.
> Ashutosh, does it match your suggestion?
>
> Anyway, here are my comments.
>
> 01.
> ```
> + int num_rows;
> ```
>
> I think num_rows is not needed, we can do like:
>
> ```
> /* Process the query result */
> - num_rows = PQntuples(res);
> - for (int i = 0; i < num_rows; i++)
> + for (int i = 0; i < PQntuples(res); i+
> ```
> And
> ```
> /* Error if no databases were found on the source server */
> - if (num_rows == 0)
> + if (num_dbs == 0)
> ```
>
Fixed.
> 02.
> ```
> + opt.all = false;
> ```
>
> This must be done after initializing recovery_timeout.
>
Fixed.
> 03.
> ```
> +# Set up node S1 as standby linking to node P
> +$node_p->backup('backup_3');
> +my $node_s1 = PostgreSQL::Test::Cluster->new('node_s1');
> ```
>
> I do not like the name "S1" because this is the second standby sever
> from the node_p.
>
Fixed.
> 04.
> ```
> +# Verify that only user databases got subscriptions (not template databases)
> +my @user_dbs = ($db1, $db2);
> +foreach my $dbname (@user_dbs)
> +{
> + my $sub_exists =
> + $node_s1->safe_psql($dbname, "SELECT count(*) FROM pg_subscription;");
> + is($sub_exists, '3', "Subscription created successfully for $dbname");
> +}
> ```
>
> Hmm, what do you want to check here? pg_subscription is a global catalog so that
> very loop will see exactly the same content. Also, 'postgres' is also the user database.
> I feel you must ensure that all three databases (postgres, $db1, and $db2) have a
> subscription here.
>
Fixed.
> 05.
> ```
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
> # Run pg_createsubscriber on node S. --verbose is used twice
> # to show more information.
> command_ok(
> ```
>
> I'm not sure the part is not needed. This have already been checked in other parts, right?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
