Enhance 'pg_createsubscriber' to retrieve databases automatically when no database is provided.
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]/messages/by-id/CAA4eK1+JpALAokLqxVsQKgo9iFrO-zChfvNXXJMkC8jUgYykBw@mail.gmail.com that was planned for
future implementation. The attached patch has the changes for the
same.
[1]: /messages/by-id/CAA4eK1+JpALAokLqxVsQKgo9iFrO-zChfvNXXJMkC8jUgYykBw@mail.gmail.com
Thanks and regards,
Shubham Khanna.
Attachments:
v1-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchapplication/octet-stream; name=v1-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchDownload+118-2
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).
--
Best Wishes,
Ashutosh Bapat
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]https://www.postgresql.org/docs/current/app-pgdump.html)
Then "all databases" would be specified something like --database = *
======
[1]: https://www.postgresql.org/docs/current/app-pgdump.html
Kind Regards,
Peter Smith.
Fujitsu Australia
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.
--
Best Wishes,
Ashutosh Bapat
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]/messages/by-id/CAExHW5sQGie7bvS-q7YUYDM2BqYZ=+xqeqFUS=cZGjK_9pnVzQ@mail.gmail.com
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]: /messages/by-id/CAExHW5sQGie7bvS-q7YUYDM2BqYZ=+xqeqFUS=cZGjK_9pnVzQ@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 30, 2025 at 6:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
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?
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.
--
With Regards,
Amit Kapila.
On Thu, Jan 30, 2025 at 12:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jan 30, 2025 at 6:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
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?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.
Thanks and regards,
Shubham Khanna.
Attachments:
v2-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchapplication/octet-stream; name=v2-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchDownload+216-3
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
On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
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.
Updated the documentation accordingly and added the current behaviour
of -a/--all-databases option.
+ /* 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?
Added a check to verify that publication and slot names are not
manually specified when using -a/--all-databases option and updated
the documentation accordingly.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachments:
v3-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchapplication/octet-stream; name=v3-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchDownload+231-3
On Wed, 5 Feb 2025 at 01:26, Shubham Khanna <khannashubham1197@gmail.com> wrote:
On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote: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.Updated the documentation accordingly and added the current behaviour
of -a/--all-databases option.+ /* 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?Added a check to verify that publication and slot names are not
manually specified when using -a/--all-databases option and updated
the documentation accordingly.The attached patch contains the suggested changes.
Hi Shubham,
Here are some of my comments:
1. We should start the comment text from the next line
+
+/* Function to validate all the databases and generate publication/slot names
+ * when using '--all-databases'.
+ */2. Here in error message it should be '--database'
+ /* 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);
+ }
+3. I think checking 'opt->all_databases' in if conditions in function
'validate_databases' is not required
+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);
+ }as we are already checking it while calling the function
+ /* Validate and process database options */
+ if (opt.all_databases)
+ validate_databases(&opt);
+4. Here we should update the count of 'num_replslots' and 'num_pubs'
+ 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;
+ }
Since we are not updating these counts, the names are reflected as expected.Should also store subscription names similarly? Or maybe store
subscription names and assign slot names same as subscription names?
5. Since --all-databases option is added we should update the comment:
/*
* If --database option is not provided, try to obtain the dbname from
* the publisher conninfo. If dbname parameter is not available, error
* out.
*/
6. Extra blank lines should be removed
}
}
+
+
/* Number of object names must match number of databases */Thanks and Regards,
Shlok Kyal
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
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.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Feb 5, 2025 at 5:31 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
On Wed, 5 Feb 2025 at 01:26, Shubham Khanna <khannashubham1197@gmail.com> wrote:
On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote: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.Updated the documentation accordingly and added the current behaviour
of -a/--all-databases option.+ /* 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?Added a check to verify that publication and slot names are not
manually specified when using -a/--all-databases option and updated
the documentation accordingly.The attached patch contains the suggested changes.
Hi Shubham,
Here are some of my comments:
1. We should start the comment text from the next line + +/* Function to validate all the databases and generate publication/slot names + * when using '--all-databases'. + */2. Here in error message it should be '--database' + /* 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); + } +3. I think checking 'opt->all_databases' in if conditions in function 'validate_databases' is not required +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); + }as we are already checking it while calling the function + /* Validate and process database options */ + if (opt.all_databases) + validate_databases(&opt); +4. Here we should update the count of 'num_replslots' and 'num_pubs'
+ 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; + } Since we are not updating these counts, the names are reflected as expected.Should also store subscription names similarly? Or maybe store
subscription names and assign slot names same as subscription names?5. Since --all-databases option is added we should update the comment:
/*
* If --database option is not provided, try to obtain the dbname from
* the publisher conninfo. If dbname parameter is not available, error
* out.
*/6. Extra blank lines should be removed
}
}+ + /* Number of object names must match number of databases */
Fixed the given comments. The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
Attachments:
v4-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchapplication/octet-stream; name=v4-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patchDownload+178-9
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 --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.
Fixed the given comments. The attached patch at [1]/messages/by-id/CAHv8Rj+DOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA@mail.gmail.com contains the
suggested changes.
[1]: /messages/by-id/CAHv8Rj+DOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA@mail.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 message1.
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.sgml5.
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.cusage:
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]/messages/by-id/CAHv8Rj+DOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA@mail.gmail.com contains the
suggested changes.
[1]: /messages/by-id/CAHv8Rj+DOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA@mail.gmail.com
Thanks and regards,
Shubham Khanna.
Dear Shubham,
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
Thanks for updates. I registered the thread to the commitfest entry [1]https://commitfest.postgresql.org/52/5566/.
Few comments.
01. fetch_source_databases
```
+ const char *query = "SELECT datname FROM pg_database WHERE datistemplate = false";
```
You told given comments were fixed, but you ignored this. Please address it
or clarify the reason why you didn't. My comment was:
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.
02. fetch_source_databases
```
+ /* Check for connection errors */
+ if (PQstatus(conn) != CONNECTION_OK)
+ {
+ pg_log_error("connection to the source server failed: %s", PQerrorMessage(conn));
+ disconnect_database(conn, false);
+ exit(1);
+ }
```connect_database() does the error handling, no need to do manually.
03. fetch_source_databases
```
+ pg_log_error("failed to execute query on the source server: %s", PQerrorMessage(conn));
```
I feel the error message is too general. Also, PQerrorMessage() is not suitable for getting error messages
generated by the query. How about:
pg_log_error("could not obtain a list of databases: %s", PQresultErrorMessage());
04. fetch_source_databases
```
+ /* 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);
+ }
```Can you clarify when this happens?
05. main
```
case 'd':
+ if (opt.all_databases)
+ {
+ pg_log_error("--database cannot be used with --all-databases");
+ exit(1);
+
```I think this erroring is not enough. getopt_long() receives options with the specified ordering, thus
-d can be accepted if the -a is specified latter.
(Same thing can be said for 'P', '--replslot' and '--subscription'.)
pg_createsubscriber ... -a -d postgres -> can be rejected,
pg_createsubscriber ... -d postgres -a -> cannot be rejected.
To avoid the issue, you must 1) add if-statements in 'a' case or 2) do validation outside of the
switch-case. I prefer 2.
[1]: https://commitfest.postgresql.org/52/5566/
Best regards,
Hayato Kuroda
FUJITSU LIMITED
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]: /messages/by-id/CAHut+PuyBsOJTSygus2-yp60sw_phwYQ-JyC+U6fCBMos9x2LA@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
