Re: speed up a logical replica setup

Поиск

Список

Период

Сортировка

От	Euler Taveira
Тема	Re: speed up a logical replica setup
Дата	29 января 04:10:45
Msg-id	d0b1534e-9f63-4966-8fb7-deedb0a626d0@app.fastmail.com обсуждение исходный текст
Ответ на	RE: speed up a logical replica setup ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>)
Ответы	RE: speed up a logical replica setup ("Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>) Re: speed up a logical replica setup ("Euler Taveira" <euler@eulerto.com>)
Список	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.