From: | "Euler Taveira" <euler(at)eulerto(dot)com> |
---|---|
To: | "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Cc: | "vignesh C" <vignesh21(at)gmail(dot)com>, "Michael Paquier" <michael(at)paquier(dot)xyz>, "Peter Eisentraut" <peter(at)eisentraut(dot)org>, "Andres Freund" <andres(at)anarazel(dot)de>, "Ashutosh Bapat" <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>, "Amit Kapila" <amit(dot)kapila16(at)gmail(dot)com>, "Shlok Kyal" <shlok(dot)kyal(dot)oss(at)gmail(dot)com> |
Subject: | Re: speed up a logical replica setup |
Date: | 2024-01-29 01:10:45 |
Message-ID: | d0b1534e-9f63-4966-8fb7-deedb0a626d0@app.fastmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Yugo NAGATA | 2024-01-29 01:52:54 | Re: Small fix on COPY ON_ERROR document |
Previous Message | Peter Smith | 2024-01-29 01:04:18 | Re: Documentation to upgrade logical replication cluster |