On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@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.
v10-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
