Re: speed up a logical replica setup - Mailing list pgsql-hackers
| From | Euler Taveira |
|---|---|
| Subject | Re: speed up a logical replica setup |
| Date | |
| Msg-id | d0b1534e-9f63-4966-8fb7-deedb0a626d0@app.fastmail.com Whole thread Raw |
| In response to | RE: speed up a logical replica setup ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) |
| Responses |
RE: speed up a logical replica setup
Re: speed up a logical replica setup |
| List | 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 thedifferent 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 thetarget 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 triggersa 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.
pgsql-hackers by date: