On Thu, Feb 20, 2025 at 4:56 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Shubham, > > Thanks for updating the patch quickly! > > > > 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. > > My point was that the loop does not have meaning because pg_subscription > is a global one. I and Peter considered changes like [1] is needed. It ensures > that each databases have a subscription. > Note: [1] cannot pass the test as-is because $db1 and $db2 contains special > characters. Please escape appropriately. >
Fixed.
> Other comments are listed in below.
>
> 01.
> ```
> + <term><option>-a</option></term>
> + <term><option>--all-dbs</option></term>
> ```
>
> Peter's comment [2] does not say that option name should be changed.
> The scope of his comment is only in the .c file.
>
Fixed.
> 02.
> ```
> +# Set up node S2 as standby linking to node P
> +$node_p->backup('backup_3');
> +my $node_s2 = PostgreSQL::Test::Cluster->new('node_s2');
> ```
>
> I feel $node_s should be renamed to $node_s1, then you can use $node_s2.
>
> Note that this change may not be needed based on the comment [3].
Fixed.
> We may have to separate the test file.
>
> [1]:
> ```
> # Verify that only user databases got subscriptions (not template databases)
> my @user_dbs = ('postgres', $db1, $db2);
> foreach my $dbname (@user_dbs)
> {
> $result =
> $node_s2->safe_psql('postgres',
> "SELECT count(*) FROM pg_subscription, pg_database
> WHERE subdbid = pg_database.oid and datname = '$dbname';");
>
> is($result, '1', "Subscription created successfully for $dbname");
> }
> ```
Fixed.
> [2]:
> https://www.postgresql.org/message-id/CAHut%2BPsatTfk9-F4JBrX_yYE0QGh4wQiTmvS4%3DdnBxcL%3DAK2HA%40mail.gmail.com
> [3]:
> https://www.postgresql.org/message-id/CAExHW5vmMs5nZ6%3DXcCYAXMJrhVrsW7hOovyg%2BP%2BT9Pkuc7DykA%40mail.gmail.com
>
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v11-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
