Thread: Re: Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.

On Fri, Jan 24, 2025 at 7:28 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 8:14 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Thu, Jan 23, 2025 at 10:33 PM Ashutosh Bapat
> > <ashutosh.bapat.oss@gmail.com> wrote:
> > >
> > > On Wed, Jan 22, 2025 at 7:29 PM Shubham Khanna
> > > <khannashubham1197@gmail.com> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > I am writing to propose an enhancement to the pg_createsubscriber
> > > > utility that enables it to automatically fetch all non-template
> > > > databases from the publisher when no specific databases are specified
> > > > by the user. This was an open item from [1] that was planned for
> > > > future implementation. The attached patch has the changes for the
> > > > same.
> > >
> > > I think the feature will be useful, but UI might cause some unwanted
> > > results. If a user forgets to specify -d option, the utility will
> > > create subscriptions to all the databases, some of which may or may
> > > not have the publications. I think it's better to provide an option to
> > > specify all databases explicitly (e.g. --all-databases).
> > >
> >
> >
> > +1 better to be safe.
> >
> > Instead of a new switch, how about changing the --database switch to
> > accept a pattern (like pg_dump --schema does [1])
> >
> > Then "all databases" would be specified something like --database = *
> >
>
> WFM but that will be more work than what's in the patch.
>

OK, what if, instead of full pattern matching it could recognise just
one special dbname value of '*' (meaning "all")

So, "all databases" could still be specified as --database = *

The implementation would be almost no more work than the current
patch, while at the same time leaving it open to be extended as a
pattern if needed in the future. Or, is it too hacky?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Jan 28, 2025 at 3:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 7:28 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2025 at 8:14 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > On Thu, Jan 23, 2025 at 10:33 PM Ashutosh Bapat
> > > <ashutosh.bapat.oss@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 22, 2025 at 7:29 PM Shubham Khanna
> > > > <khannashubham1197@gmail.com> wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > > > > I am writing to propose an enhancement to the pg_createsubscriber
> > > > > utility that enables it to automatically fetch all non-template
> > > > > databases from the publisher when no specific databases are specified
> > > > > by the user. This was an open item from [1] that was planned for
> > > > > future implementation. The attached patch has the changes for the
> > > > > same.
> > > >
> > > > I think the feature will be useful, but UI might cause some unwanted
> > > > results. If a user forgets to specify -d option, the utility will
> > > > create subscriptions to all the databases, some of which may or may
> > > > not have the publications. I think it's better to provide an option to
> > > > specify all databases explicitly (e.g. --all-databases).
> > > >
> > >
> > >
> > > +1 better to be safe.
> > >
> > > Instead of a new switch, how about changing the --database switch to
> > > accept a pattern (like pg_dump --schema does [1])
> > >
> > > Then "all databases" would be specified something like --database = *
> > >
> >
> > WFM but that will be more work than what's in the patch.
> >
>
> OK, what if, instead of full pattern matching it could recognise just
> one special dbname value of '*' (meaning "all")
>
> So, "all databases" could still be specified as --database = *
>
> The implementation would be almost no more work than the current
> patch, while at the same time leaving it open to be extended as a
> pattern if needed in the future. Or, is it too hacky?

I don't remember any precedence here. pg_dump has pg_dumpall which
dumps all the databases, so they chose to create a separate binary. If
we go that route, I think we will be able to produce a more flexible
utility, like replication slot names per database, or per database
subscription settings etc. Maybe we should consider that option.

If we want to stick to --database= supporting a pattern looks better
than just a single special pattern *.

--
Best Wishes,
Ashutosh Bapat

On Tue, Jan 28, 2025 at 12:01 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> If we want to stick to --database= supporting a pattern looks better
> than just a single special pattern *.
>

This sounds reasonable to me as well. Note that the interaction of
other parameters like --replication-slot is not yet discussed. I think
if the number of slots given matches with the number of databases
fetched based on pattern matches then we can use them otherwise,
return the ERROR. The other option could be that we don't allow
options like --replication-slot along with pattern matching option.

--
With Regards,
Amit Kapila.

On Wed, Jan 29, 2025 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Jan 28, 2025 at 12:01 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > If we want to stick to --database= supporting a pattern looks better
> > than just a single special pattern *.
> >
>
> This sounds reasonable to me as well. Note that the interaction of
> other parameters like --replication-slot is not yet discussed. I think
> if the number of slots given matches with the number of databases
> fetched based on pattern matches then we can use them otherwise,
> return the ERROR. The other option could be that we don't allow
> options like --replication-slot along with pattern matching option.
>

I have had second thoughts about my pattern idea. Now, I favour just
adding another --all-databases switch like Ashutosh had suggested [1]
in the first place.

I had overlooked the rules saying that the user is allowed to specify
*multiple* --publication or --replication-slot or --subscription name
switches, but when doing so they have to match the same number of
--database switches. Using a --dbname=pattern would be fraught with
complications. e.g. How can we know up-front how many databases the
dbname pattern will resolve to, and even in what order they get
resolved?

In hindsight, it would be much easier to just have one extra switch,
so the rules then are simple:

--all-databases  (here you CANNOT specify multiple
publication/replication-slot/subscription switches)
-database=dbname (here you CAN specify multiple
publication/replication-slot/subscription switches using the same
order and the same number as databases)

Also, --all-databases and --database switches cannot be specified at
the same time.

======
[1] https://www.postgresql.org/message-id/CAExHW5sQGie7bvS-q7YUYDM2BqYZ%3D%2BxqeqFUS%3DcZGjK_9pnVzQ%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Hi Shubham,

On Tue, Feb 4, 2025 at 2:10 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
> > >
> >
> > It could be a bit tricky to find that for users but they can devise a
> > query to get the names and numbers of databases matching the given
> > pattern.  OTOH, I am not sure there is a clear need at this stage for
> > pattern matching for this tool. So, we can go with a simple switch as
> > you are proposing at this stage.
> >
>
> After reconsidering the idea of supporting '--all-databases' switch is
> the better approach at this stage, I have added the new switch in the
> latest patch.
> The attached patch contains the suggested changes.

+ If neither <option>-d</option> nor <option>-a</option> is
+       specified, <application>pg_createsubscriber</application> will use
+       <option>--all-databases</option> by default.

As pointed upthread by me and Peter, using --all-databases by default
is not a good behaviour.

But the code doesn't behave like --all-databases by default. Looks
like we need to fix the documentation.

+ /* Generate publication and slot names if not specified */
+ SimpleStringListCell *cell;
+
+ fetch_all_databases(opt);
+
+ cell = opt->database_names.head;

We don't seem to check existence of publication and slot name
specification as the comment indicates. Do we need to check that those
names are not specified at all? and also mention in the documentation
that those specifications are required when using -a/--all-databases
option?


--
Best Wishes,
Ashutosh Bapat

Dear Shubham,

Thanks for working on it. I have some comments for the patch.

01. fetch_all_databases
```
+/* Placeholder function to fetch all databases from the publisher */
+static void
+fetch_all_databases(struct CreateSubscriberOptions *opt)
```

Please update the comment atop function.

02. fetch_all_databases
```
+    /* Establish a connection to the PostgreSQL server */
+    conn = PQconnectdb(opt->pub_conninfo_str);
+    /* Check for connection errors */
+    if (PQstatus(conn) != CONNECTION_OK)
+    {
+        pg_log_error("connection to the PostgreSQL server failed: %s", PQerrorMessage(conn));
+        PQfinish(conn);
+        exit(1);
+    }
```

You can use connect_database() instead of directly calling PQconnectdb().

03. fetch_all_databases
```
+    const char *query = "SELECT datname FROM pg_database WHERE datistemplate = false";
```

We should verify the attribute datallowconn, which indicates we can connect to the
database. If it is false, no one would be able to connect to the db and define anything.

04. fetch_all_databases
```
+    /* Check if any databases were added */
+    if (opt->database_names.head == NULL)
+    {
+        pg_log_error("no database names could be fetched or specified");
+        pg_log_error_hint("Ensure that databases exist on the publisher or specify a database using --database
option.");
+        PQclear(res);
+        PQfinish(conn);
+        exit(1);
+    }
```

It is enough to check num_rows > 0.

05. fetch_all_databases
```
+    PQfinish(conn);
```

Just in case: we can use disconnect_database().

06. validate_databases
```
+    /* Check for conflicting options */
+    if (opt->all_databases && opt->database_names.head != NULL)
+    {
+        pg_log_error("cannot specify --dbname when using --all-databases");
+        exit(1);
+    }
+
+    /*
+     * Ensure publication and slot names are not manually specified with
+     * --all-databases
+     */
+    if (opt->all_databases &&
+        (opt->pub_names.head != NULL || opt->replslot_names.head != NULL))
+    {
+        pg_log_error("cannot specify --publication or --replication-slot when using --all-databases");
+        exit(1);
+    }
```

I think validations should be done in main() like other paramters.

07. validate_databases
```
+    if (opt->all_databases)
+    {
```

This function is called when all_databases is true, so this is not needed.

08. validate_databases
```
+        cell = opt->database_names.head;
+
+        while (cell != NULL)
+        {
+            char        slot_name[NAMEDATALEN];
+
+            snprintf(slot_name, sizeof(slot_name), "%s_slot", cell->val);
+            simple_string_list_append(&opt->replslot_names, slot_name);
+
+            snprintf(slot_name, sizeof(slot_name), "%s_pub", cell->val);
+            simple_string_list_append(&opt->pub_names, slot_name);
+
+            cell = cell->next;
+        }
```

I'm not sure the part. It seems to me that you tried to set slotname to ${dbname}_slot
and pubname to ${dbname}_pub, but this the current naming rule. Since we can
generate names in setup_publisher(), I think we do not have to do something here.

09.
```
@@ -2117,6 +2233,8 @@ main(int argc, char **argv)
         }
     }
 
+
+
```

Unnesesarry blank.

10. 040_pg_createsubscriber.pl
```
+# Fetch the count of non-template databases on the publisher before
+# running pg_createsubscriber without '--all-databases' option
+my $db_count_before =
+  $node_a->safe_psql('postgres',
+    "SELECT count(*) FROM pg_database WHERE datistemplate = false;");
+is($db_count_before, '1', 'database count without --all-databases option');
```

This test is not actually realted with our codes, no need to do.

11. 040_pg_createsubscriber.pl
```
+# Ensure there are some user databases on the publisher
+$node_a->safe_psql('postgres', 'CREATE DATABASE db3');
+$node_a->safe_psql('postgres', 'CREATE DATABASE db4');
+
+$node_b->stop;
```

You must wait until all changes have been replicated to the standby here.

12. 040_pg_createsubscriber.pl

Can we do the similar tests without adding new instances? E.g., runs with
dry-run mode and confirms all databases become target.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Feb 6, 2025 at 7:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for working on it. I have some comments for the patch.
>
> 01. fetch_all_databases
> ```
> +/* Placeholder function to fetch all databases from the publisher */
> +static void
> +fetch_all_databases(struct CreateSubscriberOptions *opt)
> ```
>
> Please update the comment atop function.
>
> 02. fetch_all_databases
> ```
> +       /* Establish a connection to the PostgreSQL server */
> +       conn = PQconnectdb(opt->pub_conninfo_str);
> +       /* Check for connection errors */
> +       if (PQstatus(conn) != CONNECTION_OK)
> +       {
> +               pg_log_error("connection to the PostgreSQL server failed: %s", PQerrorMessage(conn));
> +               PQfinish(conn);
> +               exit(1);
> +       }
> ```
>
> You can use connect_database() instead of directly calling PQconnectdb().
>
> 03. fetch_all_databases
> ```
> +       const char *query = "SELECT datname FROM pg_database WHERE datistemplate = false";
> ```
>
> We should verify the attribute datallowconn, which indicates we can connect to the
> database. If it is false, no one would be able to connect to the db and define anything.
>
> 04. fetch_all_databases
> ```
> +       /* Check if any databases were added */
> +       if (opt->database_names.head == NULL)
> +       {
> +               pg_log_error("no database names could be fetched or specified");
> +               pg_log_error_hint("Ensure that databases exist on the publisher or specify a database using
--databaseoption."); 
> +               PQclear(res);
> +               PQfinish(conn);
> +               exit(1);
> +       }
> ```
>
> It is enough to check num_rows > 0.
>
> 05. fetch_all_databases
> ```
> +       PQfinish(conn);
> ```
>
> Just in case: we can use disconnect_database().
>
> 06. validate_databases
> ```
> +       /* Check for conflicting options */
> +       if (opt->all_databases && opt->database_names.head != NULL)
> +       {
> +               pg_log_error("cannot specify --dbname when using --all-databases");
> +               exit(1);
> +       }
> +
> +       /*
> +        * Ensure publication and slot names are not manually specified with
> +        * --all-databases
> +        */
> +       if (opt->all_databases &&
> +               (opt->pub_names.head != NULL || opt->replslot_names.head != NULL))
> +       {
> +               pg_log_error("cannot specify --publication or --replication-slot when using --all-databases");
> +               exit(1);
> +       }
> ```
>
> I think validations should be done in main() like other paramters.
>
> 07. validate_databases
> ```
> +       if (opt->all_databases)
> +       {
> ```
>
> This function is called when all_databases is true, so this is not needed.
>
> 08. validate_databases
> ```
> +               cell = opt->database_names.head;
> +
> +               while (cell != NULL)
> +               {
> +                       char            slot_name[NAMEDATALEN];
> +
> +                       snprintf(slot_name, sizeof(slot_name), "%s_slot", cell->val);
> +                       simple_string_list_append(&opt->replslot_names, slot_name);
> +
> +                       snprintf(slot_name, sizeof(slot_name), "%s_pub", cell->val);
> +                       simple_string_list_append(&opt->pub_names, slot_name);
> +
> +                       cell = cell->next;
> +               }
> ```
>
> I'm not sure the part. It seems to me that you tried to set slotname to ${dbname}_slot
> and pubname to ${dbname}_pub, but this the current naming rule. Since we can
> generate names in setup_publisher(), I think we do not have to do something here.
>
> 09.
> ```
> @@ -2117,6 +2233,8 @@ main(int argc, char **argv)
>                 }
>         }
>
> +
> +
> ```
>
> Unnesesarry blank.
>
> 10. 040_pg_createsubscriber.pl
> ```
> +# Fetch the count of non-template databases on the publisher before
> +# running pg_createsubscriber without '--all-databases' option
> +my $db_count_before =
> +  $node_a->safe_psql('postgres',
> +       "SELECT count(*) FROM pg_database WHERE datistemplate = false;");
> +is($db_count_before, '1', 'database count without --all-databases option');
> ```
>
> This test is not actually realted with our codes, no need to do.
>
> 11. 040_pg_createsubscriber.pl
> ```
> +# Ensure there are some user databases on the publisher
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db3');
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db4');
> +
> +$node_b->stop;
> ```
>
> You must wait until all changes have been replicated to the standby here.
>
> 12. 040_pg_createsubscriber.pl
>
> Can we do the similar tests without adding new instances? E.g., runs with
> dry-run mode and confirms all databases become target.
>

Fixed the given comments. The attached patch at [1] contains the
suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Thu, Feb 6, 2025 at 9:39 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham.
>
> Here are some review comments for v3-0001.
>
> FYI, I skipped the review of the test code because I saw Kuroda-san
> had already posted some comments about the tests
>
> ======
> Commit message
>
> 1.
> This patch enhances the 'pg_createsubscriber' utility to automatically
> fetch all non-template databases from the publisher and create subscriptions
> for them. This simplifies converting a physical standby to a logical subscriber
> for multiple databases, particularly during upgrades.
>
> ~
>
> AFAIK you are not creating the subscription at the publisher database,
> which is what this description sounds like it is saying. I think you
> are creating the subscriptions on the target server on the *same name*
> databases that you found on the source server (???). Anyway, this
> needs clarification. Also, in some of the comments below, I had the
> same confusion.
>
> ~~~
>
> 2.
> A new function 'fetch_all_databases' in 'pg_createsubscriber.c'
> that queries the publisher for all databases and adds them to the subscription
> options is added.
> Additionally, 'validate_databases' ensures that conflicting options are not
> used together, enforcing correct usage of '--all-databases'.
>
> ~
>
> IMO it is better here to describe the overall logic/algorithms rather
> than saying "This new function does this, and this other new function
> does that". (otherwise, your commit message will quickly become stale
> and need changing all the time).
>
> ~~~
>
> 3.
> The '--all-databases' option fetches all databases from the publisher.
> It auto-generates publication and replication slot names as 'pub_names' and
> 'slot_names' respectively.
>
> ~
>
> 'pub_names'?
> 'slot_names'?
>
> What are those strings? Those are not the names that you wrote in the document.
>
> ~~~
>
> 4.
> This option validates database name lengths to ensure generated names fit
> within system limits.
>
> ~
>
> No. Maybe the patch code ensures this (but I didn't find this
> validation anywhere), but certainly "This option" doesn't.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 5.
> The synopsis has no mention of --all-databases.
>
> I'm not sure of the best way to do it -- if it becomes too messy to
> combine everything then maybe having several is the way to go:
>
> e.g.
>
> pg_createsubscriber [option...] { -a | --all-databases } { -D |
> --pgdata }datadir { -P | --publisher-server }connstr
> pg_createsubscriber [option...] { -d | --database }dbname { -D |
> --pgdata }datadir { -P | --publisher-server }connstr
>
> ~~~
>
> 6.
> +      <para>
> +       Automatically fetch all non-template databases from the publisher and
> +       create subscriptions for them.
> +       When using <option>--all-databases</option>, publication and
> +       replication slot names will be automatically generated in the format
> +       <literal><replaceable>database</replaceable>_pub</literal> and
> +       <literal><replaceable>database</replaceable>_slot</literal>
> respectively.
> +       When using <option>-a</option>, ensure database names are shorter
> +       than 59 characters to allow for generated publication and slot names.
> +       replica.
> +      </para>
>
> 6a.
> "fetch all non-template databases from the publisher and create
> subscriptions for them" -- Hmm. It's too confusing. I guess you meant
> that you are enumerating all the databases on the source server but
> then creating subscriptions on all the databases at the target server
> that have the same name (???) Anyway, the current text is not clear. I
> also think it is better to use the "target server" and "source server"
> (aka publication-server) terminology.
>
> ~
>
> 6b.
> I don't think you need to say "When using
> <option>--all-databases</option>," because this entire description for
> '--all-databases'
>
> ~
>
> 6c.
> Talks about publication and slot names but no mention of subscription names?
>
> ~
>
> 6d.
> That limitation about the 59 char limit seems a strange thing to have
> documented here. I wonder why you need to say anything at all. It
> might be better if the code just validates all this up-front before
> processing and can raise an ERROR in the very rare times this might
> happen.
>
> ~
>
> 6e.
> Spurious text junk? "replica"
>
> ~
>
> 6f.
> Don't you need to also say something like "Cannot be used with
> '--publication', '--subscription', or '--replication-slot'."
>
> ~~~
>
> 7. --database
>
> -       switches.
> +       switches. If <option>-d</option> and <option>-a</option> are specified
> +       together, <application>pg_createsubscriber</application> will return
> +       an error.
>
> A nicer way to say that would be to spell it out fully. e.g. "Cannot
> be used with --all-databases'.".
>
> ~~~
>
> 8.
> For --publication don't you need to mention "Cannot be used with
> --all-databases'.".
>
> ~~~
>
> 9.
> For --subscription don't you need to mention "Cannot be used with
> --all-databases'.".
>
> ~~~
>
> 10.
> For --replication-slot don't you need to mention "Cannot be used with
> --all-databases'.".
>
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
>
> 11.
>   printf(_("\nOptions:\n"));
> + printf(_("  -a, --all-databases             fetch and specify all
> databases\n"));
>
> This doesn't really seem a helpful usage text. Should it say something
> more like "create subscriptions for all databases"... (??) **
>
> ** I have difficulty imagining what is the expected behaviour in cases
> where there might be different databases at the source server and
> target server. I think your documentation needs to have some special
> notes about this to make it clear what server you are enumerating
> these databases on, and then what happens if there is some mismatch of
> what databases are available on the *other* server.
>
> ~~~
>
> fetch_all_databases:
>
> 12.
> +/* Placeholder function to fetch all databases from the publisher */
> +static void
> +fetch_all_databases(struct CreateSubscriberOptions *opt)
> +{
>
> 12a.
> What is a "placeholder function"?
>
> ~
>
> 12b.
> I would be nicers to use the same terminology that seems more common
> in the documentation -- e.g. "source server" instead of "publisher".
> Also, the function name should make it clear if you are getting these
> data from the source server or target server.
>
> ~~~
>
> 13.
> + /* Process the query result */
> + num_rows = PQntuples(res);
> + for (int i = 0; i < num_rows; i++)
> + {
> + const char *dbname = PQgetvalue(res, i, 0);
> +
> + simple_string_list_append(&opt->database_names, dbname);
> + num_dbs++;
> + }
> The increment num_dbs++ is a bit sneaky. AFAICT you are effectively
> pretending --all-databases is the equivalent of a whole lot of
> --database so the subsequent logic won't know the difference. I think
> that logic deserves a code comment.
>
> ~~~
>
> 14.
> + /* Check if any databases were added */
> + if (opt->database_names.head == NULL)
> + {
> + pg_log_error("no database names could be fetched or specified");
> + pg_log_error_hint("Ensure that databases exist on the publisher or
> specify a database using --database option.");
> + PQclear(res);
> + PQfinish(conn);
> + exit(1);
> + }
> +
> + PQclear(res);
> + PQfinish(conn);
> +}
>
>
> 14a.
> The "were added" seems odd. I think you mean. Just "Error if no
> databases were found".
>
> ~
>
> 14b.
> Isn't it simpler just to check if num_dbs == 0?
>
> ~
>
> 14c.
> "no database names could be fetched or specified" seems like a weird
> error text. e.g. "specified"?
>
> ~
>
> 14d.
> The hint seems strange to me. If there are no databases found using
> --all-database then how will "specify a database using --database
> option." help the user get a better result?
>
> ~~~
>
> validate_databases:
>
> 15.
> +static void
> +validate_databases(struct CreateSubscriberOptions *opt)
> +{
> + /* Check for conflicting options */
> + if (opt->all_databases && opt->database_names.head != NULL)
> + {
> + pg_log_error("cannot specify --dbname when using --all-databases");
> + exit(1);
> + }
> +
> + /*
> + * Ensure publication and slot names are not manually specified with
> + * --all-databases
> + */
> + if (opt->all_databases &&
> + (opt->pub_names.head != NULL || opt->replslot_names.head != NULL))
> + {
> + pg_log_error("cannot specify --publication or --replication-slot
> when using --all-databases");
> + exit(1);
> + }
> +
>
> I think all this switch-checking is misplaced. It should be happening
> up-front in the main function.
>
> ~~~
>
> 16.
> + /* Auto-generate publication and slot names for all databases */
> + if (opt->all_databases)
> + {
> + SimpleStringListCell *cell;
> +
> + fetch_all_databases(opt);
> +
> + cell = opt->database_names.head;
> +
> + while (cell != NULL)
> + {
> + char slot_name[NAMEDATALEN];
> +
> + snprintf(slot_name, sizeof(slot_name), "%s_slot", cell->val);
> + simple_string_list_append(&opt->replslot_names, slot_name);
> +
> + snprintf(slot_name, sizeof(slot_name), "%s_pub", cell->val);
> + simple_string_list_append(&opt->pub_names, slot_name);
> +
> + cell = cell->next;
> + }
> + }
>
>
> 16a.
> Why is this code checking 'opt->all_databases'? Isn't it only possible
> to get to this function via --all-databases. You can just Assert this.
>
> ~
>
> 16b.
> Why are you reusing 'slot_name' variable like this?
>
> ~
>
> 16c.
> Was there some ERROR checking missing here to ensure the length of the
> dbname is not going to cause pub/slot to exceed NAMEDATALEN?
>
> ~
>
> 16d.
> In hindsight, most of this function seems unnecessary to me. Probably
> you could've done all this pub/slot name assignment inside the
> fetch_all_databases() at the same time as you were doing
> simple_string_list_append(&opt->database_names, dbname); for each
> database.
>
> ~~~
>
> 17.
> - while ((c = getopt_long(argc, argv, "d:D:np:P:s:t:U:v",
> + while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
>   long_options, &option_index)) != -1)
>
>
> Isn't the code within this loop missing all the early switch
> validation for checking you are not combining incompatible switches?
>
> e.g.
> --all-databases - "Cannot specify --all-databases more than once"
> --all-databases - "Cannot combine with --database, --publication,
> --subscription, --replication-slot"
> --database - "Cannot combine with --all-databases"
> --publication - "Cannot combine with --all-databases"
> --subscription - "Cannot combine with --all-databases"
> --replication-slot - "Cannot combine with --all-databases"
>
> ~~~
>
> 18.
> +
> +
>   /* Number of object names must match number of databases */
>
> Remove spurious whitespace.
>
> ======

Fixed the given comments. The attached patch at [1] contains the
suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Some review comments for v4-0001.

======
Commit message

1.
This patch enhances the 'pg_createsubscriber' utility by adding the
'--all-databases' option, which automatically fetches all non-template
databases on the source server (publisher) and creates corresponding
subscriptions on the target server (subscriber). This simplifies the process
of converting a physical standby to a logical subscriber, particularly
during upgrades.

When '--all-databases' is used, the tool queries the source server for all
databases and attempts to create subscriptions on the target server for
databases with matching names. The tool auto-generates subscription,publication
and replication slot names using the format:
  - Subscription: '<database>_sub'
  - Publication: '<database>_pub'
  - Replication slot: '<database>_slot'

~

There seems a lot of duplication in the above 2 paragraphs.
Duplication can be removed.
BTW, those generated formats are suspicious -- see a later comment below.

SUGGESTION:
This patch enhances the 'pg_createsubscriber' utility by adding the
'--all-databases' option. When '--all-databases' is specified, the tool
queries the source server (publisher) for all databases and creates
subscriptions on the target server (subscriber) for databases with matching
names. This simplifies the process of converting a physical standby to a
logical subscriber, particularly during upgrades.

The tool auto-generates subscription, publication and replication slot
names using the format:
...

~~~

2.
The patch ensures that conflicting options such as '--all-databases' and
'--database', '--publication', '--subscription', or '--replication-slot' cannot
be used together

~

That wording seems misleading because it makes it sound like
'--publication' and '--database' cannot be used together.

SUGGESTION
The options '--database', '--publication', '--subscription', and
'--replication-slot' cannot be used when '--all-databases' is
specified.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

3.
What about the synopsis (??)

v4 did not address my previous review comment [1, #5] about the synopsis

> 5.
> The synopsis has no mention of --all-databases.
>
> I'm not sure of the best way to do it -- if it becomes too messy to
> combine everything then maybe having several is the way to go:
>
> e.g.
>
> pg_createsubscriber [option...] { -a | --all-databases } { -D |
> --pgdata }datadir { -P | --publisher-server }connstr
> pg_createsubscriber [option...] { -d | --database }dbname { -D |
> --pgdata }datadir { -P | --publisher-server }connstr
>

~~~

4.
+       Automatically fetch all non-template databases from the source server
+       and create subscriptions for databases with the same names on the
+       target server.
+       If a database is present on the source but missing on the target, it is
+       skipped or an error is raised.
+       If a database exists on the target but not on the source, no
+       subscription is created for it.

"it is skipped or an error is raised" (??) So which is it? It cannot be both.

~~~

5.
+       Subscription names, publication names, and replication slot names are
+       automatically generated using the format:
+       <literal><replaceable>database</replaceable>_sub</literal>
+       <literal><replaceable>database</replaceable>_pub</literal> and
+       <literal><replaceable>database</replaceable>_slot</literal>
respectively.
+      </para>

This seems strange to me, because according to the documentation NOTES
section when --database is used and there is no
publication/subscription name etc then all the generated name format
looks like:

------
If the --publication option is not specified, the publication has the
following name pattern: “pg_createsubscriber_%u_%x” (parameter:
database oid, random int). If the --replication-slot option is not
specified, the replication slot has the following name pattern:
“pg_createsubscriber_%u_%x” (parameters: database oid, random int).
------

and

------
If the --subscription option is not specified, the subscription has
the following name pattern: “pg_createsubscriber_%u_%x” (parameters:
database oid, random int).
------

So, why are the generated names different for '--all-databases'?
Actually, I can't see any code even doing what the new documentation
is saying so is the documentation even telling the truth? The commit
message will also be impacted.

~~~

6.
+      <para>
+       <option>--all-databases</option> cannot be used with
+       <option>--publication</option>, <option>--subscription</option>,
+       or <option>--replication-slot</option>.
+      </para>

What about "--database".

~~~

7.
For all the other incompatible options you wrote:

"Cannot be used together with <option>-a</option>."

IMO it might be nioer to give the full name of the option.
e.g. "Cannot be used together with <option>--all-databases</option>."

======
src/bin/pg_basebackup/pg_createsubscriber.c

usage:

8.
+ printf(_("  -a, --all-databases             create subscriptions for
all matching databases on the target\n"));

That seems ambiguous to me. How about something more like below to
clarify the subscription is created on the target.

"for all source databases create subscriptions on the same target database"

~~~

fetch_source_databases:

9.
+ const char *query = "SELECT datname FROM pg_database WHERE
datistemplate = false";

Do we need this variable? It seems simpler/better just to put the SQL in-line.

~~~

main:

10.
  switch (c)
  {
+ case 'a':
+ opt.all_databases = true;
+ break;
+

10a.
Missing the check for duplicate option --all-databases. OTOH maybe you
don't care about this error, because probably it is harmless if you
just ignore it.

~

10b.
Missing the check for --database, --publication, --subscription,
--replication-slot.

(e.g. if user specified the --all-databases *after* those other options)

~~~

11.
+ if (opt.all_databases)
+ {
+ pg_log_error("--replslot cannot be used with --all-databases");
+ exit(1);
+ }

Where'd this name come from? AFAICT there is no such option as "--replslot"

~~~

12.
- * If --database option is not provided, try to obtain the dbname from
- * the publisher conninfo. If dbname parameter is not available, error
- * out.
+ * If neither --database nor --all-databases option is provided, try
+ * to obtain the dbname from the publisher conninfo. If dbname
+ * parameter is not available, error out.

For this comment to make sense I think the previous comment (where
fetch_source_databases is called) needs to explain that when
--all-databases is defined then it is going to treat that internally
as the equivalent of a whole lot of user-specified --database options.

======
.../t/040_pg_createsubscriber.pl

13.
+ qr/cannot specify --publication or --replication-slot when using
--all-databases/,
+ 'fail if --all-databases is used with publication and slot');
+

How are tests expecting this even passing?

Searching the patch I cannot find any such error!

======
[1] https://www.postgresql.org/message-id/CAHut%2BPuyBsOJTSygus2-yp60sw_phwYQ-JyC%2BU6fCBMos9x2LA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Mon, Feb 10, 2025 at 7:05 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Some review comments for v4-0001.
>
> ======
> Commit message
>
> 1.
> This patch enhances the 'pg_createsubscriber' utility by adding the
> '--all-databases' option, which automatically fetches all non-template
> databases on the source server (publisher) and creates corresponding
> subscriptions on the target server (subscriber). This simplifies the process
> of converting a physical standby to a logical subscriber, particularly
> during upgrades.
>
> When '--all-databases' is used, the tool queries the source server for all
> databases and attempts to create subscriptions on the target server for
> databases with matching names. The tool auto-generates subscription,publication
> and replication slot names using the format:
>   - Subscription: '<database>_sub'
>   - Publication: '<database>_pub'
>   - Replication slot: '<database>_slot'
>
> ~
>
> There seems a lot of duplication in the above 2 paragraphs.
> Duplication can be removed.
> BTW, those generated formats are suspicious -- see a later comment below.
>
> SUGGESTION:
> This patch enhances the 'pg_createsubscriber' utility by adding the
> '--all-databases' option. When '--all-databases' is specified, the tool
> queries the source server (publisher) for all databases and creates
> subscriptions on the target server (subscriber) for databases with matching
> names. This simplifies the process of converting a physical standby to a
> logical subscriber, particularly during upgrades.
>
> The tool auto-generates subscription, publication and replication slot
> names using the format:
> ...
>
> ~~~
>
> 2.
> The patch ensures that conflicting options such as '--all-databases' and
> '--database', '--publication', '--subscription', or '--replication-slot' cannot
> be used together
>
> ~
>
> That wording seems misleading because it makes it sound like
> '--publication' and '--database' cannot be used together.
>
> SUGGESTION
> The options '--database', '--publication', '--subscription', and
> '--replication-slot' cannot be used when '--all-databases' is
> specified.
>
> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 3.
> What about the synopsis (??)
>
> v4 did not address my previous review comment [1, #5] about the synopsis
>
> > 5.
> > The synopsis has no mention of --all-databases.
> >
> > I'm not sure of the best way to do it -- if it becomes too messy to
> > combine everything then maybe having several is the way to go:
> >
> > e.g.
> >
> > pg_createsubscriber [option...] { -a | --all-databases } { -D |
> > --pgdata }datadir { -P | --publisher-server }connstr
> > pg_createsubscriber [option...] { -d | --database }dbname { -D |
> > --pgdata }datadir { -P | --publisher-server }connstr
> >
>
> ~~~
>
> 4.
> +       Automatically fetch all non-template databases from the source server
> +       and create subscriptions for databases with the same names on the
> +       target server.
> +       If a database is present on the source but missing on the target, it is
> +       skipped or an error is raised.
> +       If a database exists on the target but not on the source, no
> +       subscription is created for it.
>
> "it is skipped or an error is raised" (??) So which is it? It cannot be both.
>
> ~~~
>
> 5.
> +       Subscription names, publication names, and replication slot names are
> +       automatically generated using the format:
> +       <literal><replaceable>database</replaceable>_sub</literal>
> +       <literal><replaceable>database</replaceable>_pub</literal> and
> +       <literal><replaceable>database</replaceable>_slot</literal>
> respectively.
> +      </para>
>
> This seems strange to me, because according to the documentation NOTES
> section when --database is used and there is no
> publication/subscription name etc then all the generated name format
> looks like:
>
> ------
> If the --publication option is not specified, the publication has the
> following name pattern: “pg_createsubscriber_%u_%x” (parameter:
> database oid, random int). If the --replication-slot option is not
> specified, the replication slot has the following name pattern:
> “pg_createsubscriber_%u_%x” (parameters: database oid, random int).
> ------
>
> and
>
> ------
> If the --subscription option is not specified, the subscription has
> the following name pattern: “pg_createsubscriber_%u_%x” (parameters:
> database oid, random int).
> ------
>
> So, why are the generated names different for '--all-databases'?
> Actually, I can't see any code even doing what the new documentation
> is saying so is the documentation even telling the truth? The commit
> message will also be impacted.
>
> ~~~
>
> 6.
> +      <para>
> +       <option>--all-databases</option> cannot be used with
> +       <option>--publication</option>, <option>--subscription</option>,
> +       or <option>--replication-slot</option>.
> +      </para>
>
> What about "--database".
>
> ~~~
>
> 7.
> For all the other incompatible options you wrote:
>
> "Cannot be used together with <option>-a</option>."
>
> IMO it might be nioer to give the full name of the option.
> e.g. "Cannot be used together with <option>--all-databases</option>."
>
> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> usage:
>
> 8.
> + printf(_("  -a, --all-databases             create subscriptions for
> all matching databases on the target\n"));
>
> That seems ambiguous to me. How about something more like below to
> clarify the subscription is created on the target.
>
> "for all source databases create subscriptions on the same target database"
>
> ~~~
>
> fetch_source_databases:
>
> 9.
> + const char *query = "SELECT datname FROM pg_database WHERE
> datistemplate = false";
>
> Do we need this variable? It seems simpler/better just to put the SQL in-line.
>
> ~~~
>
> main:
>
> 10.
>   switch (c)
>   {
> + case 'a':
> + opt.all_databases = true;
> + break;
> +
>
> 10a.
> Missing the check for duplicate option --all-databases. OTOH maybe you
> don't care about this error, because probably it is harmless if you
> just ignore it.
>
> ~
>
> 10b.
> Missing the check for --database, --publication, --subscription,
> --replication-slot.
>
> (e.g. if user specified the --all-databases *after* those other options)
>
> ~~~
>
> 11.
> + if (opt.all_databases)
> + {
> + pg_log_error("--replslot cannot be used with --all-databases");
> + exit(1);
> + }
>
> Where'd this name come from? AFAICT there is no such option as "--replslot"
>
> ~~~
>
> 12.
> - * If --database option is not provided, try to obtain the dbname from
> - * the publisher conninfo. If dbname parameter is not available, error
> - * out.
> + * If neither --database nor --all-databases option is provided, try
> + * to obtain the dbname from the publisher conninfo. If dbname
> + * parameter is not available, error out.
>
> For this comment to make sense I think the previous comment (where
> fetch_source_databases is called) needs to explain that when
> --all-databases is defined then it is going to treat that internally
> as the equivalent of a whole lot of user-specified --database options.
>
> ======
> .../t/040_pg_createsubscriber.pl
>
> 13.
> + qr/cannot specify --publication or --replication-slot when using
> --all-databases/,
> + 'fail if --all-databases is used with publication and slot');
> +
>
> How are tests expecting this even passing?
>
> Searching the patch I cannot find any such error!
>
> ======

Fixed the given comments. The attached patch at [1] contains the
suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8Rj%2BmhYgh8XLFM8sN8J05z0ia%2BknfB1kP6kjbnB55H-b-mw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Mon, 10 Feb 2025 at 20:36, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> The attached patch contains the suggested changes.

If a new database is created on the primary server while
pg_createsubscriber is running, the subscription will not be created
for the new database.
To reproduce this issue, follow these steps:
Debug pg_createsubscriber and set a breakpoint after the
fetch_source_databases function, where the databases would have been
prepared.
While execution of pg_createsubscriber is paused, create a new
database on the primary node.
You will observe that the database is created on the standby node, but
the subscription will not be created for the newly created database.
+fetch_source_databases(struct CreateSubscriberOptions *opt)
+{
+       PGconn     *conn;
+       PGresult   *res;
+       int                     num_rows;
+
+       /* Establish a connection to the PostgreSQL server */
+       conn = connect_database(opt->pub_conninfo_str, true);
+
+       res = PQexec(conn, "SELECT datname FROM pg_database WHERE
datistemplate = false AND datallowconn = true");
+
+       /* Check for errors during query execution */
+       if (PQresultStatus(res) != PGRES_TUPLES_OK)

Regards,
Vignesh

Hi Shubham.

Responding with a blanket statement "Fixed the given comments" makes
it difficult to be sure what was done when I come to confirm the
changes. Often embedded questions go unanswered, and if changes are
*not* made, then I can't tell if there was a good reason for rejection
or was the comment just overlooked. Please can you treat the itemised
feedback as a checklist and reply individually to each item. Even just
saying "Fixed" is useful because then I can trust the item was seen
and addressed.

e.g. From previous review [1]
#7. Not fixed. The same docs issue still exists for --publication and
for --subscription.
#13. Unanswered question "How are tests expecting this even passing?".
Was a reason identified? IOW, how can we be sure the latest tests
don't have a similar problem?

//////

Some more review comments for the patch v5-0001.

======
doc/src/sgml/ref/pg_createsubscriber.sgml

Synopsis

1.
pg_createsubscriber [option...] { -a | --all-databases } { -d |
--database }dbname { -D | --pgdata }datadir { -P | --publisher-server
}connstr

This looks misleading because '-all-database' and '--database' cannot
be combined.

I think it will be better to have 2 completely different synopsis like
I had originally suggested [2, comment #5]. E.g. just add a whole
seperate <cmdsynopsis> block adjacent to the other one in the SGML:

------
  <cmdsynopsis>
   <command>pg_createsubscriber</command>
   <arg rep="repeat"><replaceable>option</replaceable></arg>
   <group choice="plain">
    <group choice="req">
     <arg choice="plain"><option>-a</option></arg>
     <arg choice="plain"><option>--all-databases</option></arg>
    </group>
    <group choice="req">
     <arg choice="plain"><option>-D</option> </arg>
     <arg choice="plain"><option>--pgdata</option></arg>
    </group>
    <replaceable>datadir</replaceable>
    <group choice="req">
     <arg choice="plain"><option>-P</option></arg>
     <arg choice="plain"><option>--publisher-server</option></arg>
    </group>
    <replaceable>connstr</replaceable>
   </group>
  </cmdsynopsis>
------

~~~

2. --all-databases

+      <para>
+       <option>--all-databases</option> cannot be used with
+       <option>--database</option>, <option>--publication</option>,
+       <option>--replication-slot</option> or <option>--subscription</option>.
+      </para>

If you want consistent wording with the other places in this patch,
then this should just be the last sentence of the previous paragraph
and be worded like: "Cannot be used together with..."

~~~

3. --publication

-       a generated name is assigned to the publication name.
+       a generated name is assigned to the publication name. Cannot be used
+       together with <option>-a</option>.

Previously reported. Claimed fixed -a to --all-databases. Not fixed.

~~~

4. --subscription

-       a generated name is assigned to the subscription name.
+       a generated name is assigned to the subscription name. Cannot be used
+       together with <option>-a</option>.


(same as the previous review comment).

Previously reported. Claimed fixed -a to --all-databases. Not fixed.

======
src/bin/pg_basebackup/pg_createsubscriber.c

5.
+ bool all_databases; /* fetch and specify all databases */

Comment wording. "and specified" (??). Maybe just "--all-databases
option was specified"

======
[1] https://www.postgresql.org/message-id/CAHut%2BPuYmTyi6kPFnJDTvD%3DaLHd0kyX4VG6iDQVEk-ixov1AwA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAHut%2BPuyBsOJTSygus2-yp60sw_phwYQ-JyC%2BU6fCBMos9x2LA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> > #13. Unanswered question "How are tests expecting this even passing?".
> > Was a reason identified? IOW, how can we be sure the latest tests
> > don't have a similar problem?
> >
>
> In the v4-0001 patch [1], the tests were mistakenly using
> 'command_fails' instead of 'command_fails_like' to verify failed test
> cases. Since 'command_fails' only checks for a non-zero exit code and
> not specific error messages, the tests were passing even when the
> expected errors were not being triggered correctly.
> To address this, I have updated the patch to use 'command_fails_like',
> ensuring that the test cases now explicitly verify the correct failure
> messages.
>

Ah, that makes sense. Thanks for sharing the reason. So in fact, it
was a valid concern because the v5 was still carrying over the same
flaw from v4... Anyway, it is good to know it is fixed now in v6.

=====

Some general comments for the patch v6-0001:

Do you need to test every possible bad option combination? It may be
fine because the error will be immediately raised so I expect the
execution overhead to be ~zero.

BTW, your bad option combination tests are only using  --all-databases
*after* the other options. Maybe you should mix it up a bit, sometimes
putting it *before* the others as well, because rearranging will cause
different errors.

Everything else now looks good to me.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

Dear Shubham,

Thanks for updating the patch! I feel the patch has good shape. Here is a small comment.

```
+       /* Error if no databases were found on the source server */
+       if (num_rows == 0)
+       {
+               pg_log_error("no databases found on the source server");
+               pg_log_error_hint("Ensure that there are user-created databases on the source server.");
+               PQclear(res);
+               disconnect_database(conn, false);
+               exit(1);
+       }
```

I think the error message is not accurate. This error can happen when there are
user-created database but it is created as template. How about below:

```
-               pg_log_error("no databases found on the source server");
-               pg_log_error_hint("Ensure that there are user-created databases on the source server.");
+               pg_log_error("no convertable databases found on the source server");
+               pg_log_error_hint("Ensure that there are non-template and connectable databases on the source
server.");
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Feb 12, 2025 at 10:58 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > > #13. Unanswered question "How are tests expecting this even passing?".
> > > Was a reason identified? IOW, how can we be sure the latest tests
> > > don't have a similar problem?
> > >
> >
> > In the v4-0001 patch [1], the tests were mistakenly using
> > 'command_fails' instead of 'command_fails_like' to verify failed test
> > cases. Since 'command_fails' only checks for a non-zero exit code and
> > not specific error messages, the tests were passing even when the
> > expected errors were not being triggered correctly.
> > To address this, I have updated the patch to use 'command_fails_like',
> > ensuring that the test cases now explicitly verify the correct failure
> > messages.
> >
>
> Ah, that makes sense. Thanks for sharing the reason. So in fact, it
> was a valid concern because the v5 was still carrying over the same
> flaw from v4... Anyway, it is good to know it is fixed now in v6.
>

FYI, I've proposed a patch [1] that can prevent this mistake from
happening in future.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPu-umU%3DCkcSvrqL9eQt85Rv_qfLtekqU-yrjfh%3DGirUQg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia.

On Tue, 11 Feb 2025 at 15:46, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 6:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Shubham.
> >
> > Responding with a blanket statement "Fixed the given comments" makes
> > it difficult to be sure what was done when I come to confirm the
> > changes. Often embedded questions go unanswered, and if changes are
> > *not* made, then I can't tell if there was a good reason for rejection
> > or was the comment just overlooked. Please can you treat the itemised
> > feedback as a checklist and reply individually to each item. Even just
> > saying "Fixed" is useful because then I can trust the item was seen
> > and addressed.
> >
>
> I appreciate the feedback. I will ensure that each item is addressed
> individually.
>
> > e.g. From previous review [1]
> > #7. Not fixed. The same docs issue still exists for --publication and
> > for --subscription.
>
> Fixed this issue in the attached patch.
>
> > #13. Unanswered question "How are tests expecting this even passing?".
> > Was a reason identified? IOW, how can we be sure the latest tests
> > don't have a similar problem?
> >
>
> In the v4-0001 patch [1], the tests were mistakenly using
> 'command_fails' instead of 'command_fails_like' to verify failed test
> cases. Since 'command_fails' only checks for a non-zero exit code and
> not specific error messages, the tests were passing even when the
> expected errors were not being triggered correctly.
> To address this, I have updated the patch to use 'command_fails_like',
> ensuring that the test cases now explicitly verify the correct failure
> messages.
>
> > //////
> >
> > Some more review comments for the patch v5-0001.
> >
> > ======
> > doc/src/sgml/ref/pg_createsubscriber.sgml
> >
> > Synopsis
> >
> > 1.
> > pg_createsubscriber [option...] { -a | --all-databases } { -d |
> > --database }dbname { -D | --pgdata }datadir { -P | --publisher-server
> > }connstr
> >
> > This looks misleading because '-all-database' and '--database' cannot
> > be combined.
> >
> > I think it will be better to have 2 completely different synopsis like
> > I had originally suggested [2, comment #5]. E.g. just add a whole
> > seperate <cmdsynopsis> block adjacent to the other one in the SGML:
> >
> > ------
> >   <cmdsynopsis>
> >    <command>pg_createsubscriber</command>
> >    <arg rep="repeat"><replaceable>option</replaceable></arg>
> >    <group choice="plain">
> >     <group choice="req">
> >      <arg choice="plain"><option>-a</option></arg>
> >      <arg choice="plain"><option>--all-databases</option></arg>
> >     </group>
> >     <group choice="req">
> >      <arg choice="plain"><option>-D</option> </arg>
> >      <arg choice="plain"><option>--pgdata</option></arg>
> >     </group>
> >     <replaceable>datadir</replaceable>
> >     <group choice="req">
> >      <arg choice="plain"><option>-P</option></arg>
> >      <arg choice="plain"><option>--publisher-server</option></arg>
> >     </group>
> >     <replaceable>connstr</replaceable>
> >    </group>
> >   </cmdsynopsis>
> > ------
> >
> > ~~~
> >
>
> Fixed.
>
> > 2. --all-databases
> >
> > +      <para>
> > +       <option>--all-databases</option> cannot be used with
> > +       <option>--database</option>, <option>--publication</option>,
> > +       <option>--replication-slot</option> or <option>--subscription</option>.
> > +      </para>
> >
> > If you want consistent wording with the other places in this patch,
> > then this should just be the last sentence of the previous paragraph
> > and be worded like: "Cannot be used together with..."
> >
> > ~~~
> >
>
> Fixed.
>
> > 3. --publication
> >
> > -       a generated name is assigned to the publication name.
> > +       a generated name is assigned to the publication name. Cannot be used
> > +       together with <option>-a</option>.
> >
> > Previously reported. Claimed fixed -a to --all-databases. Not fixed.
> >
> > ~~~
> >
>
> Fixed.
>
> > 4. --subscription
> >
> > -       a generated name is assigned to the subscription name.
> > +       a generated name is assigned to the subscription name. Cannot be used
> > +       together with <option>-a</option>.
> >
> >
> > (same as the previous review comment).
> >
> > Previously reported. Claimed fixed -a to --all-databases. Not fixed.
> >
>
> Fixed.
>
> > ======
> > src/bin/pg_basebackup/pg_createsubscriber.c
> >
> > 5.
> > + bool all_databases; /* fetch and specify all databases */
> >
> > Comment wording. "and specified" (??). Maybe just "--all-databases
> > option was specified"
> >
> > ======
>
> Fixed.
>
> The attached Patch contains the suggested changes.
>
> [1] - https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
>

Hi Shubham,

I reviewed v6 patch, here are some of my comments:

1. Option 'P' is for "publisher-server".
            case 'P':
+               if (opt.all_databases)
+               {
+                   pg_log_error("--publication cannot be used with
--all-databases");
+                   exit(1);
+               }

So, if we provide the "--all-databases" option and then the
"--publisher-server" option. Then the command pg_createsubscriber is
failing, which is wrong.

2. In case we provide "--all-database" option and then "--publication"
option, the pg_createsubscriber command is running. I think the above
condition should be added at "case 2:" instead of "case 'P':"

Thanks and Regards,
Shlok Kyal

On Wed, Feb 12, 2025 at 6:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch! I feel the patch has good shape. Here is a small comment.
>
> ```
> +       /* Error if no databases were found on the source server */
> +       if (num_rows == 0)
> +       {
> +               pg_log_error("no databases found on the source server");
> +               pg_log_error_hint("Ensure that there are user-created databases on the source server.");
> +               PQclear(res);
> +               disconnect_database(conn, false);
> +               exit(1);
> +       }
> ```
>
> I think the error message is not accurate. This error can happen when there are
> user-created database but it is created as template. How about below:
>
> ```
> -               pg_log_error("no databases found on the source server");
> -               pg_log_error_hint("Ensure that there are user-created databases on the source server.");
> +               pg_log_error("no convertable databases found on the source server");
> +               pg_log_error_hint("Ensure that there are non-template and connectable databases on the source
server.");
> ```
>

Fixed the given comment. The attached patch at [1] contains the
suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjKc6LMJ86b4yCmwNTn0c65mz0BGMCF-vPJSKDMOGagVGA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Wed, Feb 12, 2025 at 1:15 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Wed, Feb 12, 2025 at 5:29 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> > <khannashubham1197@gmail.com> wrote:
> > >
> > > > #13. Unanswered question "How are tests expecting this even passing?".
> > > > Was a reason identified? IOW, how can we be sure the latest tests
> > > > don't have a similar problem?
> > > >
> > >
> > > In the v4-0001 patch [1], the tests were mistakenly using
> > > 'command_fails' instead of 'command_fails_like' to verify failed test
> > > cases. Since 'command_fails' only checks for a non-zero exit code and
> > > not specific error messages, the tests were passing even when the
> > > expected errors were not being triggered correctly.
> > > To address this, I have updated the patch to use 'command_fails_like',
> > > ensuring that the test cases now explicitly verify the correct failure
> > > messages.
> > >
> >
> > Ah, that makes sense. Thanks for sharing the reason. So in fact, it
> > was a valid concern because the v5 was still carrying over the same
> > flaw from v4... Anyway, it is good to know it is fixed now in v6.
> >
> > =====
> >
> > Some general comments for the patch v6-0001:
> >
> > Do you need to test every possible bad option combination? It may be
> > fine because the error will be immediately raised so I expect the
> > execution overhead to be ~zero.
> >
> > BTW, your bad option combination tests are only using  --all-databases
> > *after* the other options. Maybe you should mix it up a bit, sometimes
> > putting it *before* the others as well, because rearranging will cause
> > different errors.
> >
> > Everything else now looks good to me.
>

Fixed the comment given by Shlok at [1]. The attached patch at [2]
contains the suggested changes.

[1] - https://www.postgresql.org/message-id/CANhcyEXGwFeQd2fuB2txm1maCC2zKyROUR5exEMGzPYdrZ4CPQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAHv8RjKc6LMJ86b4yCmwNTn0c65mz0BGMCF-vPJSKDMOGagVGA%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Some review comments for v7-0001

======
src/bin/pg_basebackup/pg_createsubscriber.c

1.
+ /* Error if no databases were found on the source server */
+ if (num_rows == 0)
+ {
+ pg_log_error("no convertable databases found on the source server");
+ pg_log_error_hint("Ensure that there are non-template and
connectable databases on the source server.");
+ PQclear(res);
+ disconnect_database(conn, false);
+ exit(1);
+ }

1a.
The spelling is "convertible", not "convertable"

~

1b.
What does "convertible databases" mean here, and will that term make
sense to a user? How are we "converting" the source server databases;
AFAIK we are simply connecting to them and adding publications.

IMO a better choice of adjectives can be found below.
"no suitable databases found..."
"no appropriate databases found..."
"no eligible databases found..."
etc.

======
Kind Regards,
Peter Smith.
Fujitsu Australia.

Hi Shubham,
Here are some review comments from my side


On Thu, Feb 13, 2025 at 8:58 AM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
> The attached patch contains the required changes.
>

clusterdb, vacuumdb use -a and -all for all databases. Do we want to
use the same long option name here? Probably they don't have
-databases in the long option name since db is already in their name.
pg_createsubscriber doesn't have db in its name. But still it might be
better to have just -all to be consistent with those utilities. Sorry
if this has already been discussed.

+ printf(_(" -a, --all-databases create subscriptions on the target
for all source databases\n"));

Suggest "create subscriptions for all non-template source databases".
"on the target" seems unnecessary, it's implicit.

+
+# run pg_createsubscriber with '--database' and '--all-databases' and
verify the
+# failure
+command_fails_like(
+ [
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--all-databases',
+ ],
+ qr/--all-databases cannot be used with --database/,
+ 'fail if --all-databases is used with --database');

Need a test where --all-databases is specified before --database.

All the tests run in --dry-mode which won't test whether all the
replication slots, subscriptions and publications are working well
when --all-databases is specified. I think we need to test non-dry
mode creation. I may go as far as suggesting to split the current test
file into 3, 1. tests sanity of command line options 2. tests single
subscription creation thoroughly along with multiple --database
specifications 3. tests --all-databases health of the subscriptions
and --all-databases specific scenarios like non-template databases
aren't subscribed to.

--
Best Wishes,
Ashutosh Bapat

On Mon, 10 Feb 2025 at 22:14, vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 10 Feb 2025 at 20:36, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > The attached patch contains the suggested changes.
>
> If a new database is created on the primary server while
> pg_createsubscriber is running, the subscription will not be created
> for the new database.
> To reproduce this issue, follow these steps:
> Debug pg_createsubscriber and set a breakpoint after the
> fetch_source_databases function, where the databases would have been
> prepared.
> While execution of pg_createsubscriber is paused, create a new
> database on the primary node.
> You will observe that the database is created on the standby node, but
> the subscription will not be created for the newly created database.
> +fetch_source_databases(struct CreateSubscriberOptions *opt)
> +{
> +       PGconn     *conn;
> +       PGresult   *res;
> +       int                     num_rows;
> +
> +       /* Establish a connection to the PostgreSQL server */
> +       conn = connect_database(opt->pub_conninfo_str, true);
> +
> +       res = PQexec(conn, "SELECT datname FROM pg_database WHERE
> datistemplate = false AND datallowconn = true");
> +
> +       /* Check for errors during query execution */
> +       if (PQresultStatus(res) != PGRES_TUPLES_OK)

If we don't have a solution for this, how about the following approach:
a) Add a note in the documentation indicating that the publication and
subscriptions must be created manually for any new databases created
while pg_createsubscriber is running. b) At the end of the
pg_createsubscriber execution, identify any new databases that were
created and list them for the user.

Since this scenario might not be common, we could either implement
just option a) or both a) and b).

Regards,
Vignesh

Dear Vignesh,

> > If a new database is created on the primary server while
> > pg_createsubscriber is running, the subscription will not be created
> > for the new database.
> > To reproduce this issue, follow these steps:
> > Debug pg_createsubscriber and set a breakpoint after the
> > fetch_source_databases function, where the databases would have been
> > prepared.
> > While execution of pg_createsubscriber is paused, create a new
> > database on the primary node.
> > You will observe that the database is created on the standby node, but
> > the subscription will not be created for the newly created database.
> > +fetch_source_databases(struct CreateSubscriberOptions *opt)
> > +{
> > +       PGconn     *conn;
> > +       PGresult   *res;
> > +       int                     num_rows;
> > +
> > +       /* Establish a connection to the PostgreSQL server */
> > +       conn = connect_database(opt->pub_conninfo_str, true);
> > +
> > +       res = PQexec(conn, "SELECT datname FROM pg_database WHERE
> > datistemplate = false AND datallowconn = true");
> > +
> > +       /* Check for errors during query execution */
> > +       if (PQresultStatus(res) != PGRES_TUPLES_OK)
> 
> If we don't have a solution for this, how about the following approach:
> a) Add a note in the documentation indicating that the publication and
> subscriptions must be created manually for any new databases created
> while pg_createsubscriber is running. b) At the end of the
> pg_createsubscriber execution, identify any new databases that were
> created and list them for the user.
> 
> Since this scenario might not be common, we could either implement
> just option a) or both a) and b).

I'm not sure we must really handle the case. Documentation [1] has already described
not to run DDL commands during the command:

```
Since DDL commands are not replicated by logical replication, avoid executing DDL
commands that change the database schema while running pg_createsubscriber.
If the target server has already been converted to logical replica, the DDL commands
might not be replicated, which might cause an error.
```

Thought?

[1]: https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Shubham,

Thanks for updating the patch. Few comments.

```
+       If a database is present on the source but missing on the target, an
+       error is raised.
```

I'm not sure this description is accurate. I feel there is a case the command can
succeed.

Assuming that there are three databases (db1, db2, and db3) on the primary but db3
is missing on the standby when DBA is running the command. In check_subscriber(),
pg_createsubscriber connects to the db1 and it says OK. Then we wait until all
changes are replicated, thus db3 is created on the target. Everything may go well afterward.

Possible handling is as follows:

1) Ignore the corner case and retain,
2) Remove the description,
3) Modify the description, or
4) Modify codes to reject when the speficied database is missing on the target.

I feel 2) may be enough, but I want to ask others as well.


```
+       If a database exists on the target but not on the source, no
+       subscription is created for it.
```

I do not think this is needed. If the database exists on the target but not on the
source, IIUC this means the DROP DATABASE was executed on the primary server but
not replicated to the standby yet. In this case such databases would be dropped
during the command because we ensure all changes are replicated before the promotion.
Tell me if there are other situations...

```
+               disconnect_database(conn, false);
+               exit(1);
```

This combination can be changed to disconnect_database(conn, true).

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thursday, February 13, 2025 11:28 AM Shubham Khanna <khannashubham1197@gmail.com> wrote:

Hi,

> The attached patch contains the required changes.

Thanks for updating the patch. I think it's a useful feature.

Here are few review comments:

1.

+                if (opt.all_databases)
+                {
+                    pg_log_error("--all-databases specified multiple times");
+                    exit(1);
+                }

The check for redundant --all-databases usage seems unnecessary as multiple
specifications do not cause harm or confusion for users. Similar server
commands with an --all option (such as clusterdb/vacuumdb/reindexdb) do not
report errors for it.

2.
+    while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
                             long_options, &option_index)) != -1)
...
+                if (num_dbs > 0)
+                {
+                    pg_log_error("--all-databases cannot be used with --database");
+                    exit(1);
+                }


I think the check for similar wrong combinations should not be done inside the
getopt_long() block. It should be performed after parsing to ensure all
cases are handled which would also be consistent with the methodology followed in
all other commands AFAICS.

Maybe we can write the error message in the format "%s cannot be used with %s"
to reduce the translation workload. 

To be consistent with other error message for wrong option specification in this
command, consider adding pg_log_error_hint("Try \"%s --help\" for more information.").

3.

Ashutosh noted that commands like clusterdb and vacuumdb use the "--all" option
to indicate all databases, and I also found that the same is true for
pg_amcheck command, so I also think it's OK to use "-all" here.

Best Regards,
Hou zj

On Tue, 18 Feb 2025 at 06:22, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> I'm not sure we must really handle the case. Documentation [1] has already described
> not to run DDL commands during the command:
>
> ```
> Since DDL commands are not replicated by logical replication, avoid executing DDL
> commands that change the database schema while running pg_createsubscriber.
> If the target server has already been converted to logical replica, the DDL commands
> might not be replicated, which might cause an error.
> ```

Yes, that makes sense. Since the same applies to tables created while
pg_createsubscriber is running, and there is already generic
documentation covering this, I agree with you.

Regards,
Vignesh

On Tue, Feb 18, 2025 at 7:32 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch. Few comments.
>
> ```
> +       If a database is present on the source but missing on the target, an
> +       error is raised.
> ```
>
> I'm not sure this description is accurate. I feel there is a case the command can
> succeed.
>
> Assuming that there are three databases (db1, db2, and db3) on the primary but db3
> is missing on the standby when DBA is running the command. In check_subscriber(),
> pg_createsubscriber connects to the db1 and it says OK. Then we wait until all
> changes are replicated, thus db3 is created on the target. Everything may go well afterward.
>
> Possible handling is as follows:
>
> 1) Ignore the corner case and retain,
> 2) Remove the description,
> 3) Modify the description, or
> 4) Modify codes to reject when the speficied database is missing on the target.
>
> I feel 2) may be enough, but I want to ask others as well.
>

I agree with you on this. Since there are cases where the command can
still succeed even if a database is initially missing on the target,
the description could be misleading. Based on this, I have removed
that part of the description.

>
> ```
> +       If a database exists on the target but not on the source, no
> +       subscription is created for it.
> ```
>
> I do not think this is needed. If the database exists on the target but not on the
> source, IIUC this means the DROP DATABASE was executed on the primary server but
> not replicated to the standby yet. In this case such databases would be dropped
> during the command because we ensure all changes are replicated before the promotion.
> Tell me if there are other situations...
>

I agree with your reasoning. Since any databases that exist on the
target but not on the source would be dropped as part of ensuring all
changes are replicated before promotion, explicitly mentioning this
scenario isn’t necessary. Based on this, I have removed that part of
the description.

> ```
> +               disconnect_database(conn, false);
> +               exit(1);
> ```
>
> This combination can be changed to disconnect_database(conn, true).
>

Fixed.

The attached patch at [1] contains the suggested changes.
[1] - https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

On Tue, Feb 18, 2025 at 2:43 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, February 13, 2025 11:28 AM Shubham Khanna <khannashubham1197@gmail.com> wrote:
>
> Hi,
>
> > The attached patch contains the required changes.
>
> Thanks for updating the patch. I think it's a useful feature.
>
> Here are few review comments:
>
> 1.
>
> +                               if (opt.all_databases)
> +                               {
> +                                       pg_log_error("--all-databases specified multiple times");
> +                                       exit(1);
> +                               }
>
> The check for redundant --all-databases usage seems unnecessary as multiple
> specifications do not cause harm or confusion for users. Similar server
> commands with an --all option (such as clusterdb/vacuumdb/reindexdb) do not
> report errors for it.
>

I agree with your point. Since multiple specifications of '--all' do
not cause any issues or confusion, and other similar commands (like
clusterdb, vacuumdb, and reindexdb) do not enforce this restriction, I
have removed the test case accordingly.

> 2.
> +       while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
>                                                         long_options, &option_index)) != -1)
> ...
> +                               if (num_dbs > 0)
> +                               {
> +                                       pg_log_error("--all-databases cannot be used with --database");
> +                                       exit(1);
> +                               }
>
>
> I think the check for similar wrong combinations should not be done inside the
> getopt_long() block. It should be performed after parsing to ensure all
> cases are handled which would also be consistent with the methodology followed in
> all other commands AFAICS.
>

I agree with your suggestion. To ensure consistency with other
commands, I have moved the check for conflicting options outside the
getopt_long() block. This allows us to handle all cases after parsing
is complete.

> Maybe we can write the error message in the format "%s cannot be used with %s"
> to reduce the translation workload.
>

Fixed.

> To be consistent with other error message for wrong option specification in this
> command, consider adding pg_log_error_hint("Try \"%s --help\" for more information.").
>

Fixed.

> 3.
>
> Ashutosh noted that commands like clusterdb and vacuumdb use the "--all" option
> to indicate all databases, and I also found that the same is true for
> pg_amcheck command, so I also think it's OK to use "-all" here.
>

Fixed.

The attached patch at [1] contains the suggested changes.
[1] - https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Dear Shubham,

Thanks for updating the patch!

> I have added a test case for non-dry-run mode to ensure that
> replication slots, subscriptions, and publications work as expected
> when '--all' is specified. Additionally, I have split the test file
> into two parts:
> 1) Command-line sanity checks – Validates the correctness of option parsing.
> 2) '--all' functionality and behavior – Verifies the health of
> subscriptions and ensures that --all specific scenarios, such as
> non-template databases not being subscribed to, are properly handled.
> This should improve test coverage while keeping the structure manageable.

TBH, I feel your change does not separate the test file into the two parts. ISTM
you just added some validation checks and a test how --all option works.
Ashutosh, does it match your suggestion?

Anyway, here are my comments.

01.
```
+    int            num_rows;
```

I think num_rows is not needed, we can do like:

```
        /* Process the query result */
-       num_rows = PQntuples(res);
-       for (int i = 0; i < num_rows; i++)
+       for (int i = 0; i < PQntuples(res); i+
```
And
```
        /* Error if no databases were found on the source server */
-       if (num_rows == 0)
+       if (num_dbs == 0)
```

02.
```
+    opt.all = false;
```

This must be done after initializing recovery_timeout.

03.
```
+# Set up node S1 as standby linking to node P
+$node_p->backup('backup_3');
+my $node_s1 = PostgreSQL::Test::Cluster->new('node_s1');
```

I do not like the name "S1" because this is the second standby sever
from the node_p.

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.

05.
```
+# Verify replication is working
+$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
+
+$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
+is( $result, qq(first row
+replication test
+second row), "replication successful in $db1");
+
+$node_s1->stop;
 # Run pg_createsubscriber on node S.  --verbose is used twice
 # to show more information.
 command_ok(
```

I'm not sure the part is not needed. This have already been checked in other parts, right?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Hi Shubham, Here are some review comments for v9-0001.

======
Commit message

1.
You've changed the option to '--all' but the comment message still
refers to '-all-databases'

======
doc/src/sgml/ref/pg_createsubscriber.sgml

2.
+      <para>
+       Automatically fetch all non-template databases from the source server
+       and create subscriptions for databases with the same names on the
+       target server.

I don't see what is "automatic" about this, nor do I know really what
"fetch" of databases is trying to convey.

Isn't it simpler to just say like below?

SUGGESTION
For all source server non-template databases create subscriptions for...

======
src/bin/pg_basebackup/pg_createsubscriber.c

3.
  int recovery_timeout; /* stop recovery after this time */
+ bool all; /* --all option was specified */
 };


IMO 'all' is too generic for a field name; it has almost no meaning --
all what? I think the original 'all_databases' name was better. Or
'all_dbs', but not just 'all'.

~~~

4.
+ if (opt.all)
+ {
+ if (num_dbs > 0)
+ {
+ pg_log_error("%s cannot be used with %s", "--all", "--database");
+ pg_log_error_hint("Try \"%s --help\" for more information.", progname);
+ exit(1);
+ }
+
+ if (num_pubs > 0)
+ {
+ pg_log_error("%s cannot be used with %s", "--all", "--publication");
+ pg_log_error_hint("Try \"%s --help\" for more information.", progname);
+ exit(1);
+ }
+
+ if (num_replslots > 0)
+ {
+ pg_log_error("%s cannot be used with %s", "--all", "--replication-slot");
+ pg_log_error_hint("Try \"%s --help\" for more information.", progname);
+ exit(1);
+ }
+
+ if (num_subs > 0)
+ {
+ pg_log_error("%s cannot be used with %s", "--all", "--subscription");
+ pg_log_error_hint("Try \"%s --help\" for more information.", progname);
+ exit(1);
+ }

4a
If bad combinations exist then IMO it is most likely that the --all
came before the incompatible option. So the parameters should be the
other way around so the result is "--<XXX> cannot be used with --all".

~

4b.
You could eliminate all this code duplication by using a variable for
the bad option,

SUGGESTION
char *bad_switch = NULL;

if (num_dbs > 0)            bad_switch = "--database";
else if (num_pubs > 0)      bad_switch = "--publication";
else if (num_replslots > 0) bad_switch = "--replication_slot";
else if (num_subs > 0)      bad_switch = "--subscription";

if (bad_switch)
{
  pg_log_error("%s cannot be used with --all", bad_switch);
  pg_log_error_hint("Try \"%s --help\" for more information.", progname);
  exit(1);
}

~

4c.
There is a double-blank line after this code fragment.

======
.../t/040_pg_createsubscriber.pl

5.
+# run pg_createsubscriber with '--database' and '--all' and verify the
+# failure
+command_fails_like(
+ [
+ 'pg_createsubscriber',
+ '--verbose',
+ '--dry-run',
+ '--pgdata' => $node_s->data_dir,
+ '--publisher-server' => $node_p->connstr($db1),
+ '--socketdir' => $node_s->host,
+ '--subscriber-port' => $node_s->port,
+ '--database' => $db1,
+ '--all',
+ ],
+ qr/--all cannot be used with --database/,
+ 'fail if --all is used with --database');
+

There is no difference anymore in the logic/error if the options are
given in order "--all --database"  or if they are in order "--database
--all". So you no longer need to have separate test cases for
different ordering.

~~~

6.
+# 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");
+}

AFAICT this is just checking a global subscription count in a loop so
it will be 3, 3, 3. Why?

I guess you intended to verify that relevant subscriptions were
created for each of the target databases. But this code is not doing
that.

~~~

7.
+# Verify replication is working
+$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
+
+$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
+is( $result, qq(first row
+replication test
+second row), "replication successful in $db1");
+
+$node_s1->stop;
 # Run pg_createsubscriber on node S.  --verbose is used twice
 # to show more information.
 command_ok(
@@ -431,8 +590,9 @@ $result = $node_s->safe_psql($db1,
 is($result, qq(0), 'failover slot was removed');

 # Check result in database $db1
-$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1');
+$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1 ORDER BY 1');
 is( $result, qq(first row
+replication test

I concur with Kuroda-san's comment. Maybe remove all this, because
AFAICT existing code was already testing that replication is working
ok for $db1 and $db2.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch!
>
> > I have added a test case for non-dry-run mode to ensure that
> > replication slots, subscriptions, and publications work as expected
> > when '--all' is specified. Additionally, I have split the test file
> > into two parts:
> > 1) Command-line sanity checks – Validates the correctness of option parsing.
> > 2) '--all' functionality and behavior – Verifies the health of
> > subscriptions and ensures that --all specific scenarios, such as
> > non-template databases not being subscribed to, are properly handled.
> > This should improve test coverage while keeping the structure manageable.
>
> TBH, I feel your change does not separate the test file into the two parts. ISTM
> you just added some validation checks and a test how --all option works.
> Ashutosh, does it match your suggestion?
>
> Anyway, here are my comments.
>
> 01.
> ```
> +       int                     num_rows;
> ```
>
> I think num_rows is not needed, we can do like:
>
> ```
>         /* Process the query result */
> -       num_rows = PQntuples(res);
> -       for (int i = 0; i < num_rows; i++)
> +       for (int i = 0; i < PQntuples(res); i+
> ```
> And
> ```
>         /* Error if no databases were found on the source server */
> -       if (num_rows == 0)
> +       if (num_dbs == 0)
> ```
>

Fixed.

> 02.
> ```
> +       opt.all = false;
> ```
>
> This must be done after initializing recovery_timeout.
>

Fixed.

> 03.
> ```
> +# Set up node S1 as standby linking to node P
> +$node_p->backup('backup_3');
> +my $node_s1 = PostgreSQL::Test::Cluster->new('node_s1');
> ```
>
> I do not like the name "S1" because this is the second standby sever
> from the node_p.
>

Fixed.

> 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.

> 05.
> ```
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
>  # Run pg_createsubscriber on node S.  --verbose is used twice
>  # to show more information.
>  command_ok(
> ```
>
> I'm not sure the part is not needed. This have already been checked in other parts, right?
>

Fixed.

The attached patch contains the suggested changes.

Thanks and regards,
Shubham Khanna.

On Thu, Feb 20, 2025 at 1:23 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Shubham, Here are some review comments for v9-0001.
>
> ======
> Commit message
>
> 1.
> You've changed the option to '--all' but the comment message still
> refers to '-all-databases'
>

Fixed.

> ======
> doc/src/sgml/ref/pg_createsubscriber.sgml
>
> 2.
> +      <para>
> +       Automatically fetch all non-template databases from the source server
> +       and create subscriptions for databases with the same names on the
> +       target server.
>
> I don't see what is "automatic" about this, nor do I know really what
> "fetch" of databases is trying to convey.
>
> Isn't it simpler to just say like below?
>
> SUGGESTION
> For all source server non-template databases create subscriptions for...
>

Fixed.

> ======
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 3.
>   int recovery_timeout; /* stop recovery after this time */
> + bool all; /* --all option was specified */
>  };
>
>
> IMO 'all' is too generic for a field name; it has almost no meaning --
> all what? I think the original 'all_databases' name was better. Or
> 'all_dbs', but not just 'all'.
>
> ~~~

Fixed.

>
> 4.
> + if (opt.all)
> + {
> + if (num_dbs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--database");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_pubs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--publication");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_replslots > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--replication-slot");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
> +
> + if (num_subs > 0)
> + {
> + pg_log_error("%s cannot be used with %s", "--all", "--subscription");
> + pg_log_error_hint("Try \"%s --help\" for more information.", progname);
> + exit(1);
> + }
>
> 4a
> If bad combinations exist then IMO it is most likely that the --all
> came before the incompatible option. So the parameters should be the
> other way around so the result is "--<XXX> cannot be used with --all".
>
> ~

Fixed.

>
> 4b.
> You could eliminate all this code duplication by using a variable for
> the bad option,
>
> SUGGESTION
> char *bad_switch = NULL;
>
> if (num_dbs > 0)            bad_switch = "--database";
> else if (num_pubs > 0)      bad_switch = "--publication";
> else if (num_replslots > 0) bad_switch = "--replication_slot";
> else if (num_subs > 0)      bad_switch = "--subscription";
>
> if (bad_switch)
> {
>   pg_log_error("%s cannot be used with --all", bad_switch);
>   pg_log_error_hint("Try \"%s --help\" for more information.", progname);
>   exit(1);
> }
>
> ~
>

Fixed.

> 4c.
> There is a double-blank line after this code fragment.
>

Fixed.

> ======
> .../t/040_pg_createsubscriber.pl
>
> 5.
> +# run pg_createsubscriber with '--database' and '--all' and verify the
> +# failure
> +command_fails_like(
> + [
> + 'pg_createsubscriber',
> + '--verbose',
> + '--dry-run',
> + '--pgdata' => $node_s->data_dir,
> + '--publisher-server' => $node_p->connstr($db1),
> + '--socketdir' => $node_s->host,
> + '--subscriber-port' => $node_s->port,
> + '--database' => $db1,
> + '--all',
> + ],
> + qr/--all cannot be used with --database/,
> + 'fail if --all is used with --database');
> +
>
> There is no difference anymore in the logic/error if the options are
> given in order "--all --database"  or if they are in order "--database
> --all". So you no longer need to have separate test cases for
> different ordering.
>
> ~~~

Fixed.

>
> 6.
> +# 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");
> +}
>
> AFAICT this is just checking a global subscription count in a loop so
> it will be 3, 3, 3. Why?
>
> I guess you intended to verify that relevant subscriptions were
> created for each of the target databases. But this code is not doing
> that.
>
> ~~~

Fixed.

>
> 7.
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
>  # Run pg_createsubscriber on node S.  --verbose is used twice
>  # to show more information.
>  command_ok(
> @@ -431,8 +590,9 @@ $result = $node_s->safe_psql($db1,
>  is($result, qq(0), 'failover slot was removed');
>
>  # Check result in database $db1
> -$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1');
> +$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1 ORDER BY 1');
>  is( $result, qq(first row
> +replication test
>
> I concur with Kuroda-san's comment. Maybe remove all this, because
> AFAICT existing code was already testing that replication is working
> ok for $db1 and $db2.
>
> ======

Fixed.

The attached patch at [1] contains the suggested changes.

[1] - https://www.postgresql.org/message-id/CAHv8RjLtgrA9odXtwhit1mUfqogNSF4qhkvDrPbxEoWba%2B4SOw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

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.

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.

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].
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");
}
```
[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

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Thu, Feb 20, 2025 at 10:26 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@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

On Fri, Feb 21, 2025 at 5:18 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:26 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@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. :)

--
Best Wishes,
Ashutosh Bapat

On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shubham,
>
> Thanks for updating the patch!
>
> > I have added a test case for non-dry-run mode to ensure that
> > replication slots, subscriptions, and publications work as expected
> > when '--all' is specified. Additionally, I have split the test file
> > into two parts:
> > 1) Command-line sanity checks – Validates the correctness of option parsing.
> > 2) '--all' functionality and behavior – Verifies the health of
> > subscriptions and ensures that --all specific scenarios, such as
> > non-template databases not being subscribed to, are properly handled.
> > This should improve test coverage while keeping the structure manageable.
>
> TBH, I feel your change does not separate the test file into the two parts. ISTM
> you just added some validation checks and a test how --all option works.
> Ashutosh, does it match your suggestion?
>

Thanks Hayato for pointing it out. The test changes don't match my
expectations. As you rightly pointed out, I expected two (actually
three if needed) separate test files one for argument validation and
one for testing --database scenarios (the existing tests) and one more
for testing same scenarios when --all is specified. Right now all it
does is "# Verify that only user databases got subscriptions (not
template databases)". I expected testing the actual replication as
well like tests between lines around 527 and 582 in the latest
patchset. Those tests are performed when --database is subscribed. We
need similar tests performed when --all is specified. I didn't find
any of those tests being performed on node_s2. Given that the tests
for --databases and --all will be very similar, having them in the
same test file makes more sense. We also seem to be missing
$node_s2->teardown_node, do we?

--
Best Wishes,
Ashutosh Bapat

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

On Fri, Feb 21, 2025 at 5:18 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:26 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@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.
>

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.
>
> 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.
>

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.

On Fri, Feb 21, 2025 at 9:44 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Fri, Feb 21, 2025 at 5:18 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 10:26 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@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.

On Fri, Feb 21, 2025 at 10:18 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Wed, Feb 19, 2025 at 4:23 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shubham,
> >
> > Thanks for updating the patch!
> >
> > > I have added a test case for non-dry-run mode to ensure that
> > > replication slots, subscriptions, and publications work as expected
> > > when '--all' is specified. Additionally, I have split the test file
> > > into two parts:
> > > 1) Command-line sanity checks – Validates the correctness of option parsing.
> > > 2) '--all' functionality and behavior – Verifies the health of
> > > subscriptions and ensures that --all specific scenarios, such as
> > > non-template databases not being subscribed to, are properly handled.
> > > This should improve test coverage while keeping the structure manageable.
> >
> > TBH, I feel your change does not separate the test file into the two parts. ISTM
> > you just added some validation checks and a test how --all option works.
> > Ashutosh, does it match your suggestion?
> >
>
> Thanks Hayato for pointing it out. The test changes don't match my
> expectations. As you rightly pointed out, I expected two (actually
> three if needed) separate test files one for argument validation and
> one for testing --database scenarios (the existing tests) and one more
> for testing same scenarios when --all is specified. Right now all it
> does is "# Verify that only user databases got subscriptions (not
> template databases)". I expected testing the actual replication as
> well like tests between lines around 527 and 582 in the latest
> patchset. Those tests are performed when --database is subscribed. We
> need similar tests performed when --all is specified. I didn't find
> any of those tests being performed on node_s2. Given that the tests
> for --databases and --all will be very similar, having them in the
> same test file makes more sense. We also seem to be missing
> $node_s2->teardown_node, do we?
>

I agree with your point. Right now, my focus is on fixing the patch
first, and I plan to split the test files at the end. That approach
might help streamline the process.
As of the latest patch attached at [1], this issue remains unresolved.
I will proceed with splitting the tests once all other issues are
addressed.

[1] - https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com

Thanks and regards,
Shubham Khanna.

Dear Shubham,

Thanks for updating the patch.

I think the modification [1] is not correct - the loop is meaningless because the same
query would be executed every time. How about idea like attached? Here, instead of
try escaping dbname, dbname is directly obtained from the instance and they are compared.

How do you think?

[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 datistemplate = 'f';"
+    );
+    is($result, '3', "Subscription created successfully for $dbname");
+    $result = $node_s2->safe_psql('postgres',
+        "SELECT count(*) FROM pg_subscription, pg_database WHERE subdbid = pg_database.oid and datistemplate = 't';"
+    );
+    is($result, '0', "Subscription created successfully for $dbname");
+}
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

On Fri, Feb 28, 2025 at 11:34 PM Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
>
> The attached Patch contains the suggested changes.
>
> Thanks and regards,
> Shubham Khanna.

Some comments:
1.
+    <varlistentry>
+     <term><option>-a</option></term>
+     <term><option>--all</option></term>
+     <listitem>
+      <para>
+       For all source server non-template databases create subscriptions for
+       create subscriptions for databases with the same names on the
+       target server.
+       Subscription names, publication names, and replication slot names are
+       automatically generated. Cannot be used together with
+       <option>--database</option>, <option>--publication</option>,
+       <option>--replication-slot</option> or <option>--subscription</option>.

Don't start the sentence with "Cannot". Change the sentence to "This
option cannot be used together with ..."
similar sentences used in 3 other places below this as well. Change all of them.

2.
+# Verify that only user databases got subscriptions (not template databases)

change to "Verify that only user databases have subscriptions"

regards,
Ajin Cherian
Fujitsu Australia