public inbox for [email protected]
help / color / mirror / Atom feedFrom: Euler Taveira <[email protected]>
To: Peter Eisentraut <[email protected]>
To: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: FabrÃzio de Royes Mello <[email protected]>
Subject: Re: speed up a logical replica setup
Date: Mon, 19 Feb 2024 20:28:28 -0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<TY3PR01MB9889F3C46DC76CE77B91FB58F5432@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<TY3PR01MB98894646568B37FE11E9CC0DF5432@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<[email protected]>
<TY3PR01MB98896B904BB259B559622773F5422@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<[email protected]>
<TYCPR01MB120779A0CC2083C1DA8534BA7F5452@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<TYCPR01MB12077835F614EF829BB035120F5442@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<TYCPR01MB120777DED2C72F897EF6E57D1F54E2@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<TYCPR01MB12077646F91423005D657FF20F54D2@TYCPR01MB12077.jpnprd01.prod.outlook.com>
<[email protected]>
<[email protected]>
On Mon, Feb 19, 2024, at 6:47 AM, Peter Eisentraut wrote:
> Some review of the v21 patch:
Thanks for checking.
> - 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.
Right. Done.
> - doc/src/sgml/ref/allfiles.sgml
>
> Move the new entry to alphabetical order.
Done.
>
> - 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" ?
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'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.
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.sgml
>
> Move the new entry to alphabetical order.
Done.
> - src/bin/pg_basebackup/Makefile
>
> Move the new sections to alphabetical order.
Done.
> - src/bin/pg_basebackup/meson.build
>
> Move the new sections to alphabetical order.
Done.
>
> - 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.)
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.pl
>
> These 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 = debug2
>
> Is 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.
[1] https://www.postgresql.org/message-id/TYCPR01MB12077A8421685E5515DE408EEF5512%40TYCPR01MB12077.jpnpr...
--
Euler Taveira
EDB https://www.enterprisedb.com/
view thread (39+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: speed up a logical replica setup
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox