Re: speed up a logical replica setup - Mailing list pgsql-hackers
| From | Euler Taveira |
|---|---|
| Subject | Re: speed up a logical replica setup |
| Date | |
| Msg-id | 0404dab2-9475-42e3-ae39-f8a71ec1d889@app.fastmail.com Whole thread Raw |
| In response to | Re: speed up a logical replica setup (Peter Eisentraut <peter@eisentraut.org>) |
| List | pgsql-hackers |
On Mon, Feb 19, 2024, at 6:47 AM, Peter Eisentraut wrote:
Some review of the v21 patch:
Thanks for checking.
- commit messageMention pg_createsubscriber in the commit message title. That's themost important thing that someone doing git log searches in the futurewill be looking for.
Right. Done.
- doc/src/sgml/ref/allfiles.sgmlMove the new entry to alphabetical order.
Done.
- doc/src/sgml/ref/pg_createsubscriber.sgml+ <para>+ The <application>pg_createsubscriber</application> should be run atthe target+ server. The source server (known as publisher server) should acceptlogical+ replication connections from the target server (known as subscriberserver).+ The target server should accept local logical replication connection.+ </para>"should" -> "must" ?
Done.
+ <refsect1>+ <title>Options</title>Sort options alphabetically.
Done.
It would be good to indicate somewhere which options are mandatory.
I'll add this information in the option description. AFAICT the synopsis kind
of indicates it.
+ <refsect1>+ <title>Examples</title>I suggest including a pg_basebackup call into this example, so it'seasier for readers to get the context of how this is supposed to beused. You can add that pg_basebackup in this example is just an exampleand that other base backups can also be used.
We can certainly add it but creating a standby isn't out of scope here? I will
make sure to include references to pg_basebackup and the "Setting up a Standby
Server" section.
- doc/src/sgml/reference.sgmlMove the new entry to alphabetical order.
Done.
- src/bin/pg_basebackup/MakefileMove the new sections to alphabetical order.
Done.
- src/bin/pg_basebackup/meson.buildMove the new sections to alphabetical order.
Done.
- src/bin/pg_basebackup/pg_createsubscriber.c+typedef struct CreateSubscriberOptions+typedef struct LogicalRepInfoI think these kinds of local-use struct don't need to be typedef'ed.(Then you also don't need to update typdefs.list.)
Done.
+static void+usage(void)Sort the options alphabetically.
Are you referring to s/options/functions/?
+static char *+get_exec_path(const char *argv0, const char *progname)Can this not use find_my_exec() and find_other_exec()?
It is indeed using it. I created this function because it needs to run the same
code path twice (pg_ctl and pg_resetwal).
+int+main(int argc, char **argv)Sort the options alphabetically (long_options struct, getopt_long()argument, switch cases).
Done.
- .../t/040_pg_createsubscriber.pl- .../t/041_pg_createsubscriber_standby.plThese two files could be combined into one.
Done.
+# Force it to initialize a new cluster instead of copying a+# previously initdb'd cluster.Explain why?
Ok. It needs a new cluster because it will have a different system identifier
so we can make sure the target cluster is a copy of the source server.
+$node_s->append_conf(+ 'postgresql.conf', qq[+log_min_messages = debug2Is this setting necessary for the test?
No. It is here as a debugging aid. Better to include it in a separate patch.
There are a few messages that I don't intend to include in the final patch.
All of these modifications will be included in the next patch. I'm finishing to
integrate patches proposed by Hayato [1] and some additional fixes and
refactors.
pgsql-hackers by date: