Some review of the v21 patch:
- commit message
Mention pg_createsubscriber in the commit message title. That's the
most important thing that someone doing git log searches in the future
will be looking for.
- doc/src/sgml/ref/allfiles.sgml
Move the new entry to alphabetical order.
- doc/src/sgml/ref/pg_createsubscriber.sgml
+ <para>
+ The <application>pg_createsubscriber</application> should be run at
the target
+ server. The source server (known as publisher server) should accept
logical
+ replication connections from the target server (known as subscriber
server).
+ The target server should accept local logical replication connection.
+ </para>
"should" -> "must" ?
+ <refsect1>
+ <title>Options</title>
Sort options alphabetically.
It would be good to indicate somewhere which options are mandatory.
+ <refsect1>
+ <title>Examples</title>
I suggest including a pg_basebackup call into this example, so it's
easier for readers to get the context of how this is supposed to be
used. You can add that pg_basebackup in this example is just an example
and that other base backups can also be used.
- doc/src/sgml/reference.sgml
Move the new entry to alphabetical order.
- src/bin/pg_basebackup/Makefile
Move the new sections to alphabetical order.
- src/bin/pg_basebackup/meson.build
Move the new sections to alphabetical order.
- src/bin/pg_basebackup/pg_createsubscriber.c
+typedef struct CreateSubscriberOptions
+typedef struct LogicalRepInfo
I think these kinds of local-use struct don't need to be typedef'ed.
(Then you also don't need to update typdefs.list.)
+static void
+usage(void)
Sort the options alphabetically.
+static char *
+get_exec_path(const char *argv0, const char *progname)
Can this not use find_my_exec() and find_other_exec()?
+int
+main(int argc, char **argv)
Sort the options alphabetically (long_options struct, getopt_long()
argument, switch cases).
- .../t/040_pg_createsubscriber.pl
- .../t/041_pg_createsubscriber_standby.pl
These two files could be combined into one.
+# Force it to initialize a new cluster instead of copying a
+# previously initdb'd cluster.
Explain why?
+$node_s->append_conf(
+ 'postgresql.conf', qq[
+log_min_messages = debug2
Is this setting necessary for the test?