On Thu, Feb 20, 2025 at 10:26 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.
Yes. Some test is still needed to confirm the expected subscriptions
all get created for respective dbs. But, the current test loop just
isn't doing it properly.
>
> 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.
Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
referring only to the field name of struct CreateSubscriberOptions,
nothing else. Not the usage help, not the error messages, not the
docs, not the tests, not the commit message.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
