RE: speed up a logical replica setup

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Cc: vignesh C <vignesh21@gmail.com>, Michael Paquier <michael@paquier.xyz>, Peter Eisentraut <peter@eisentraut.org>, Andres Freund <andres@anarazel.de>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, 'Euler Taveira' <euler@eulerto.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: 2024-01-26T07:55:51Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →
  1. pg_createsubscriber: Remove obsolete comment

  2. pg_createsubscriber: Fix an unpredictable recovery wait time.

  3. Fix unstable test in 040_pg_createsubscriber.

  4. Fix the testcase introduced in commit 81d20fbf7a.

  5. Further weaken new pg_createsubscriber test on Windows.

  6. Temporarily(?) weaken new pg_createsubscriber test on Windows.

  7. Make pg_createsubscriber warn if publisher has two-phase commit enabled.

  8. Make pg_createsubscriber more wary about quoting connection parameters.

  9. pg_createsubscriber: Remove failover replication slots on subscriber

  10. pg_createsubscriber: Remove replication slot check on primary

  11. pg_createsubscriber: Only --recovery-timeout controls the end of recovery process

  12. pg_createsubscriber: creates a new logical replica from a standby server

  13. Add some const decorations

  14. Add option force_initdb to PostgreSQL::Test::Cluster:init()

  15. Remove MSVC scripts

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/