RE: speed up a logical replica setup - Mailing list pgsql-hackers

Dear Euler,

Thanks for updating the patch!

>v24-0003: as I said I don't think we need to add it, however, I won't fight
>against it if people want to add this check.

OK, let's wait comments from senior members.

>Since I applied v24-0004, I realized that extra start / stop service are
>required. It mean pg_createsubscriber doesn't start the transformation with the
>current standby settings. Instead, it stops the standby if it is running and
>start it with the provided command-line options (socket, port,
>listen_addresses). It has a few drawbacks:
>* See v34-0012. It cannot detect if the target server is a primary for another
>  server. It is documented.

Yeah, It is a collateral damage.

>* I also removed the check for standby is running. If the standby was stopped a
>  long time ago, it will take some time to reach the start point.
>* Dry run mode has to start / stop the service to work correctly. Is it an
>  issue?

One concern (see below comment) is that -l option would not be passed even if
the standby has been logging before running pg_createsubscriber. Also, some settings
passed by pg_ctl start -o .... would not be restored.

>However, I decided to include --retain option, I'm thinking about to remove it.
>If the logging is enabled, the information during the pg_createsubscriber will
>be available. The client log can be redirected to a file for future inspection.

Just to confirm - you meant to say like below, right? 
* the client output would be redirected, and
* -r option would be removed.

Here are my initial comments for v25-0001. I read new doc and looks very good.
I may do reviewing more about v25-0001, but feel free to revise.

01. cleanup_objects_atexit
```
    PGconn       *conn;
    int            i;
```
The declaration *conn can be in the for-loop. Also, the declaration of the indicator can be in the bracket.

02. cleanup_objects_atexit
```

                /*
                 * If a connection could not be established, inform the user
                 * that some objects were left on primary and should be
                 * removed before trying again.
                 */
                if (dbinfo[i].made_publication)
                {
                    pg_log_warning("There might be a publication \"%s\" in database \"%s\" on primary",
                                   dbinfo[i].pubname, dbinfo[i].dbname);
                    pg_log_warning_hint("Consider dropping this publication before trying again.");
                }
                if (dbinfo[i].made_replslot)
                {
                    pg_log_warning("There might be a replication slot \"%s\" in database \"%s\" on primary",
                                   dbinfo[i].subname, dbinfo[i].dbname);
                    pg_log_warning_hint("Drop this replication slot soon to avoid retention of WAL files.");
                }
```

Not sure which is better, but we may able to the list to the concrete file like pg_upgrade.
(I thought it had been already discussed, but could not find from the archive. Sorry if it was a duplicated comment)

03. main
```
    while ((c = getopt_long(argc, argv, "d:D:nP:rS:t:v",
                            long_options, &option_index)) != -1)
```

Missing update for __shortopts.

04. main
```
            case 'D':
                opt.subscriber_dir = pg_strdup(optarg);
                canonicalize_path(opt.subscriber_dir);
                break;
...
            case 'P':
                opt.pub_conninfo_str = pg_strdup(optarg);
                break;
...
            case 's':
                opt.socket_dir = pg_strdup(optarg);
                break;
...
            case 'U':
                opt.sub_username = pg_strdup(optarg);
                break;
```

Should we consider the case these options would be specified twice?
I.e., should we call pg_free() before the substitution?
 
05. main

Missing canonicalize_path() to the socket_dir.

06. main
```
    /*
     * If socket directory is not provided, use the current directory.
     */
```

One-line comment can be used. Period can be also removed at that time.

07. main
```
    /*
     *
     * If subscriber username is not provided, check if the environment
     * variable sets it. If not, obtain the operating system name of the user
     * running it.
     */
```
Unnecessary blank.

08. main
```
        char       *errstr = NULL;
```
 
This declaration can be at else-part.

09. main.

Also, as the first place, do we have to get username if not specified?
I felt libpq can handle the case if we skip passing the info.

10. main
```
    appendPQExpBuffer(sub_conninfo_str, "host=%s port=%u user=%s fallback_application_name=%s",
                      opt.socket_dir, opt.sub_port, opt.sub_username, progname);
    sub_base_conninfo = get_base_conninfo(sub_conninfo_str->data, NULL);
```

Is it really needed to call get_base_conninfo? I think no need to define
sub_base_conninfo.

11. main

```
    /*
     * In dry run mode, the server is restarted with the provided command-line
     * options so validation can be applied in the target server. In order to
     * preserve the initial state of the server (running), start it without
     * the command-line options.
     */
    if (dry_run)
        start_standby_server(&opt, pg_ctl_path, NULL, false);
```

I think initial state of the server may be stopped. Now both conditions are allowed.
And I think it is not good not to specify the logfile.

12. others

As Peter E pointed out [1], the main function is still huge. It has more than 400 lines.
I think all functions should have less than 100 line to keep the readability.

I considered separation idea like below. Note that this may require to change
orderings. How do you think?

* add parse_command_options() which accepts user options and verifies them
* add verification_phase() or something which checks system identifier and calls check_XXX
* add catchup_phase() or something which creates a temporary slot, writes recovery parameters,
  and wait until the end of recovery
* add cleanup_phase() or something which removes primary_slot and modifies the
  system identifier
* stop/start server can be combined into one wrapper.

Attached txt file is proofs the concept.

13. others

PQresultStatus(res) is called 17 times in this source code, it may be redundant.
I think we can introduce a function like executeQueryOrDie() and gather in one place.

14. others

I found that pg_createsubscriber does not refer functions declared in other files.
Is there a possibility to use them, e.g., streamutils.h?

15. others 

While reading the old discussions [2], Amit suggested to keep the comment and avoid
creating a temporary slot. You said "Got it" but temp slot still exists.
Is there any reason? Can you clarify your opinion?

16. others

While reading [2] and [3], I was confused the decision. You and Amit discussed
the combination with pg_createsubscriber and slot sync and how should handle
slots on the physical standby. You seemed to agree to remove such a slot, and
Amit also suggested to raise an ERROR. However, you said in [8] that such
handlings is not mandatory so should raise an WARNING in dry_run. I was quite confused.
Am I missing something?

17. others

Per discussion around [4], we might have to consider an if the some options like
data_directory and config_file was initially specified for standby server. Another
easy approach is to allow users to specify options like -o in pg_upgrade [5],
which is similar to your idea. Thought?


18. others

How do you handle the reported failure [6]?

19. main
```
    char       *pub_base_conninfo = NULL;
    char       *sub_base_conninfo = NULL;
    char       *dbname_conninfo = NULL;
```

No need to initialize pub_base_conninfo and sub_base_conninfo.
These variables would not be free'd.

20. others

IIUC, slot creations would not be finished if there are prepared transactions.
Should we detect it on the verification phase and raise an ERROR?

21. others

As I said in [7], the catch up would not be finished if long recovery_min_apply_delay
is used. Should we overwrite during the catch up?

22. pg_createsubscriber.sgml
```
    <para>
     Check
     Write recovery parameters into the target data...
```

Not sure, but "Check" seems not needed.

[1]: https://www.postgresql.org/message-id/b9aa614c-84ba-a869-582f-8d5e3ab57424%40enterprisedb.com
[2]: https://www.postgresql.org/message-id/9fd3018d-0e5f-4507-aee6-efabfb5a4440%40app.fastmail.com
[3]: https://www.postgresql.org/message-id/CAA4eK1L%2BE-bdKaOMSw-yWizcuprKMyeejyOwWjq_57%3DUqh-f%2Bg%40mail.gmail.com
[4]:
https://www.postgresql.org/message-id/TYCPR01MB12077B63D81B49E9DFD323661F55A2%40TYCPR01MB12077.jpnprd01.prod.outlook.com
[5]:
https://www.postgresql.org/docs/devel/pgupgrade.html#:~:text=options%20to%20be%20passed%20directly%20to%20the%20old%20postgres%20command%3B%20multiple%20option%20invocations%20are%20appended
[6]: https://www.postgresql.org/message-id/CAHv8Rj%2B5mzK9Jt%2B7ECogJzfm5czvDCCd5jO1_rCx0bTEYpBE5g%40mail.gmail.com
[7]:
https://www.postgresql.org/message-id/OS3PR01MB98828B15DD9502C91E0C50D7F57D2%40OS3PR01MB9882.jpnprd01.prod.outlook.com
[8]: https://www.postgresql.org/message-id/be92c57b-82e1-4920-ac31-a8a04206db7b%40app.fastmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/global/