Dear Euler,
Again, thanks for updating the patch! There are my random comments for v9.
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...)
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,
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?
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.
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.
06.
```
+ atexit(cleanup_objects_atexit);
```
The registration of the cleanup function is too early. This sometimes triggers
a core-dump. E.g.,
```
$ pg_subscriber --publisher-conninfo --subscriber-conninfo 'user=postgres
port=5432' --verbose --database 'postgres' --pgdata data_N2/
pg_subscriber: error: too many command-line arguments (first is "user=postgres
port=5432")
pg_subscriber: hint: Try "pg_subscriber --help" for more information.
Segmentation fault (core dumped)
$ gdb ...
(gdb) bt
#0 cleanup_objects_atexit () at pg_subscriber.c:131
#1 0x00007fb982cffce9 in __run_exit_handlers () from /lib64/libc.so.6
#2 0x00007fb982cffd37 in exit () from /lib64/libc.so.6
#3 0x00000000004054e6 in main (argc=9, argv=0x7ffc59074158) at
pg_subscriber.c:1500
(gdb) f 3
#3 0x00000000004054e6 in main (argc=9, argv=0x7ffc59074158) at
pg_subscriber.c:1500
1500 exit(1);
(gdb) list
1495 if (optind < argc)
1496 {
1497 pg_log_error("too many command-line arguments (first is
\"%s\")",
1498 argv[optind]);
1499 pg_log_error_hint("Try \"%s --help\" for more
information.", progname);
1500 exit(1);
1501 }
1502
1503 /*
1504 * Required arguments
```
I still think it should be done just before the creation of objects [3].
07.
Missing a removal of publications on the standby.
08.
Missing registration of LogicalRepInfo in the typedefs.list.
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.
10.
Just to confirm - will you implement start_standby/stop_standby functions in
next version?
[1]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[2]:
https://www.postgresql.org/message-id/CAEG8a3%2BwL_2R8n12BmRz7yBP3EBNdHDhmdgxQFA9vS%2BzPR%2B3Kw%40mail.gmail.com
[3]:
https://www.postgresql.org/message-id/TY3PR01MB9889678E47B918F4D83A6FD8F57B2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
[4]:
https://www.postgresql.org/message-id/TY3PR01MB9889C362FF76102C88FA1C29F56F2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/
