Re: speed up a logical replica setup

On Fri, Jan 26, 2024, at 4:55 AM, Hayato Kuroda (Fujitsu) wrote:
> Again, thanks for updating the patch! There are my random comments for v9.

Thanks for checking v9. I already incorporated some of the points below into
the next patch. Give me a couple of hours to include some important points.

> 01.
> I cannot find your replies for my comments#7 [1] but you reverted related changes.
> I'm not sure you are still considering it or you decided not to include changes.
> Can you clarify your opinion?
> (It is needed because changes are huge so it quite affects other developments...)

It is still on my list. As I said in a previous email I'm having a hard time
reviewing pieces from your 0002 patch because you include a bunch of things
into one patch.

> 02.
> ```
> +       <term><option>-t <replaceable class="parameter">seconds</replaceable></option></term>
> +       <term><option>--timeout=<replaceable class="parameter">seconds</replaceable></option></term>
> ```
> 
> But source codes required `--recovery-timeout`. Please update either of them,

Oops. Fixed. My preference is --recovery-timeout because someone can decide to
include a --timeout option for this tool.

> 03.
> ```
> + *    Create a new logical replica from a standby server
> ```
> 
> Junwang pointed out to change here but the change was reverted [2]
> Can you clarify your opinion as well?

I'll review the documentation once I fix the code. Since the tool includes the
*create* verb into its name, it seems strange use another verb (convert) in the
description. Maybe we should just remove the word *new* and a long description
explains the action is to turn the standby (physical replica) into a logical
replica.

> 04.
> ```
> +/*
> + * Is the source server ready for logical replication? If so, create the
> + * publications and replication slots in preparation for logical replication.
> + */
> +static bool
> +setup_publisher(LogicalRepInfo *dbinfo)
> ```
> 
> But this function verifies the source server. I felt they should be in the
> different function.

I split setup_publisher() into check_publisher() and setup_publisher().

> 05.
> ```
> +/*
> + * Is the target server ready for logical replication?
> + */
> +static bool
> +setup_subscriber(LogicalRepInfo *dbinfo)
> ````
> 
> Actually, this function does not set up subscriber. It just verifies whether the
> target can become a subscriber, right? If should be renamed.

I renamed setup_subscriber() -> check_subscriber().

> 06.
> ```
> +    atexit(cleanup_objects_atexit);
> ```
> 
> The registration of the cleanup function is too early. This sometimes triggers
> a core-dump. E.g., 

I forgot to apply the atexit() patch.

> 07.
> Missing a removal of publications on the standby.

It was on my list to do. It will be in the next patch.

> 08.
> Missing registration of LogicalRepInfo in the typedefs.list.

Done.

> 09
> ```
> + <refsynopsisdiv>
> +  <cmdsynopsis>
> +   <command>pg_subscriber</command>
> +   <arg rep="repeat"><replaceable>option</replaceable></arg>
> +  </cmdsynopsis>
> + </refsynopsisdiv>
> ```
> 
> Can you reply my comment#2 [4]? I think mandatory options should be written.

I included the mandatory options into the synopsis.

> 10.
> Just to confirm - will you implement start_standby/stop_standby functions in next version?

It is still on my list.


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