On Fri, Feb 21, 2025 at 9:44 AM Ashutosh Bapat
<ashutosh.bapat....@gmail.com> wrote:
>
> On Fri, Feb 21, 2025 at 5:18 AM Peter Smith <smithpb2...@gmail.com> wrote:
> >
> > 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.
>
> +1. We don't want yet another option to specify all databases. :)
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
