public inbox for [email protected]
help / color / mirror / Atom feedFrom: Euler Taveira <[email protected]>
To: [email protected] <[email protected]>
To: [email protected] <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Shlok Kyal <[email protected]>
Subject: Re: speed up a logical replica setup
Date: Sun, 28 Jan 2024 22:10:45 -0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <TY3PR01MB98895A551923953B3DA3C7C8F5792@TY3PR01MB9889.jpnprd01.prod.outlook.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CALDaNm098Jkbh+ye6zMj9Ro9j1bBe6FfPV80BFbs1=pUuTJ07g@mail.gmail.com>
<[email protected]>
<CAA4eK1KtGC-v_CYd8yaCwisrQvoZN=+As90p9hBM29DAxA+Pmg@mail.gmail.com>
<CANhcyEUdYCR9oSt4HsZRqYZZVXsb4x-HoPGqDSxSeA2SYz-YGg@mail.gmail.com>
<CAA4eK1KMQNMhSPmsRdrCQpuc=Tsc08h6=XkLNZ5F35Q2REom6g@mail.gmail.com>
<CANhcyEXjOcnOJKdOfK8Z1+bbymuZzwSjrppbt4FKHbrDD_t7Mg@mail.gmail.com>
<CANhcyEUCt-g4JLQU3Q3ofFk_Vt-Tqh3ZdXoLcpT8fjz9LY_-ww@mail.gmail.com>
<[email protected]>
<CAA4eK1JRgnhv_ySzuFjN7UaX9qxz5Hqcwew7Fk=+AbJbu0Kd9w@mail.gmail.com>
<TY3PR01MB98895BA6C1D72CB8582CACC4F5682@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<[email protected]>
<TY3PR01MB9889C362FF76102C88FA1C29F56F2@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<TY3PR01MB9889C5D55206DDD978627D07F5752@TY3PR01MB9889.jpnprd01.prod.outlook.com>
<CANhcyEXhpfWgc0-FdHmUG6WGD=2cxYqBePr1yxJ8VSZ0gV3XKA@mail.gmail.com>
<[email protected]>
<[email protected]>
<TY3PR01MB98895A551923953B3DA3C7C8F5792@TY3PR01MB9889.jpnprd01.prod.outlook.com>
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/
view thread (97+ 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], [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