Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided. - Mailing list pgsql-hackers

On Mon, Mar 24, 2025 at 5:41 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Mon, 24 Mar 2025 at 15:56, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > On Mon, Mar 24, 2025 at 12:29 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > Dear Shubham,
> > >
> > > > I have reviewed and merged the proposed changes into the patch. The
> > > > attached patches contain the suggested changes.
> > >
> > > Thanks for updating the patch! Few comments:
> > >
> > > 01.
> > > ```
> > > +       /*
> > > +        * Fetch all databases from the source (publisher) if --all is specified.
> > > +        */
> > > +       if (opt.all_dbs)
> > > +               fetch_source_databases(&opt);
> > > +
> > >         if (opt.database_names.head == NULL)
> > > ```
> > >
> > > I feel we can convert "if" -> "else if" for the opt.database_names.head case,
> > > because fetch_source_databases() ensures databases are listed.
> > >
> >
> > Fixed.
> >
> > > 02.
> > > ```
> > > +# Set up node U as standby linking to node
> > > ```
> > >
> > > To confirm: why can we use "U" here?
> > >
> >
> > Since node_s and node_t are already used in this test, I have used the
> > next letter, node_u. Additionally, comments have been added to improve
> > clarity.
> >
> > > 03.
> > > ```
> > > +$node_u->append_conf(
> > > +       'postgresql.conf', qq[
> > > +primary_conninfo = '$pconnstr dbname=postgres'
> > > +]);
> > > ```
> > > I think this part is not required. init_from_backup() with has_streaming sets the
> > > primary_conninfo.
> > >
> >
> > Fixed.
> >
> > > 04.
> > > ```
> > > +# Stop $node_s
> > > +$node_s->stop;
> > > ```
> > >
> > > The comment does not describe anything. I feel you can say "Stop node S because
> > > it won't be used anymore", or just remove it.
> > >
> >
> > Fixed.
> >
> > > 05.
> > > ```
> > > +# Drop one of the databases (e.g., $db2)
> > > +$node_p->safe_psql('postgres', "DROP DATABASE \"$db2\"");
> > > ```
> > >
> > > "e.g." means "for example", so it is not suitable to put here. Please explain why it must be removed.
> > >
> >
> > Fixed.
> >
> > > 06.
> > > ```
> > > +# Get subscription names
> > > +$result = $node_u->safe_psql(
> > > +       'postgres', qq(
> > > +       SELECT subname FROM pg_subscription WHERE subname ~ '^pg_createsubscriber_'
> > > +));
> > > +my @subnames1 = split("\n", $result);
> > > +
> > > +# Wait subscriber to catch up
> > > +$node_u->wait_for_subscription_sync($node_p, $subnames1[0]);
> > > +$node_u->wait_for_subscription_sync($node_p, $subnames1[1]);
> > > ```
> > >
> > > wait_for_subscription_sync() is used to wait until the initial synchronization is
> > > done, so not suitable here. wait_for_catchup() is more appropriate.
> > >
> >
> > Fixed.
> >
> > > 07.
> > > Also, the similar part exists on the pre-existing part (wait_for_subscription_sync
> > > was used there, but I feel this may not be correct). How about unifing them like
> > > attached?
> > >
> >
> > Fixed.
> >
> > > 08.
> > > ```
> > > +# Verify logical replication works for all databases
> > > +# Insert rows on node P
> > > +$node_p->safe_psql('postgres', "INSERT INTO tbl1 VALUES('first row')");
> > > ```
> > >
> > > This is confusing because 'fourth row' is inserted to $db1 just after it. How
> > > about the 'row in database postgres' or something?
> > >
> >
> > Fixed.
> >
> > The attached patches contain the suggested changes.
> >
>
> I have reviewed the v19-0001 patch I have a comment:
>
> In pg_createsubscriber.sgml, this line seems little odd:
>
> +       obtained from <option>-P</option> option. If the database name is not
> +       specified in either the <option>-d</option> option, or the
> +       <option>-P</option> option, and <option>-a</option>
> +       an error will be reported.
>
> Should we change the sentence to something like:
>
> +       obtained from <option>-P</option> option. If the database name is not
> +       specified in either the <option>-d</option> option, or the
> +       <option>-P</option> option, and <option>-a</option> option is not
> +       specified, an error will be reported.
>

Fixed.

The attached patches contain the suggested changes.

Thanks and regards,
Shubham Khanna.