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
