Re: speed up a logical replica setup

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.jpnprd01.prod.outlook.com

--
Euler Taveira
EDB https://www.enterprisedb.com/