On Thu, Feb 20, 2025 at 4:56 PM Hayato Kuroda (Fujitsu)
<kuroda.hay...@fujitsu.com> 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
