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

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.

Thanks,
Shlok Kyal