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

Thanks for updating the patch!

>

I'm still working on the data structures to group options. I don't like the way

it was grouped in v13-0005. There is too many levels to reach database name.

The setup_subscriber() function requires the 3 data structures.

>

Right, your refactoring looks fewer stack. So I pause to revise my refactoring

patch.

>

The documentation update is almost there. I will include the modifications in

the next patch.

>

OK. I think it should be modified before native speakers will attend to the

thread.

>

Regarding v13-0004, it seems a good UI that's why I wrote a comment about it.

However, it comes with a restriction that requires a similar HBA rule for both

regular and replication connections. Is it an acceptable restriction? We might

paint ourselves into the corner. A reasonable proposal is not to remove this

option. Instead, it should be optional. If it is not provided, primary_conninfo

is used.

>

I didn't have such a point of view. However, it is not related whether -P exists

or not. Even v14-0001 requires primary to accept both normal/replication connections.

If we want to avoid it, the connection from pg_createsubscriber can be restored

to replication-connection.

(I felt we do not have to use replication protocol even if we change the connection mode)

The motivation why -P is not needed is to ensure the consistency of nodes.

pg_createsubscriber assumes that the -P option can connect to the upstream node,

but no one checks it. Parsing two connection strings may be a solution but be

confusing. E.g., what if some options are different?

I think using a same parameter is a simplest solution.

I found that no one refers the name of temporary slot. Can we remove the variable?

Initialization by `CreateSubscriberOptions opt = {0};` seems enough.

All values are set to 0x0.

You said "target server must be a standby" in [1], but I cannot find checks for it.

IIUC, there are two approaches:

a) check the existence "standby.signal" in the data directory

b) call an SQL function "pg_is_in_recovery"

I still think they can be combined as "bindir".

WriteRecoveryConfig() writes GUC parameters to postgresql.auto.conf, but not

sure it is good. These settings would remain on new subscriber even after the

pg_createsubscriber. Can we avoid it? I come up with passing these parameters

via pg_ctl -o option, but it requires parsing output from GenerateRecoveryConfig()

(all GUCs must be allign like "-c XXX -c XXX -c XXX...").

Functions arguments should not be struct because they are passing by value.

They should be a pointer. Or, for modify_subscriber_sysid and wait_for_end_recovery,

we can pass a value which would be really used.

07.

```

static char *get_base_conninfo(char *conninfo, char *dbname,

   const char *noderole);

```

Not sure noderole should be passed here. It is used only for the logging.

Can we output string before calling the function?

(The parameter is not needed anymore if -P is removed)

08.

The terminology is still not consistent. Some functions call the target as standby,

but others call it as subscriber.

09.

v14 does not work if the standby server has already been set recovery_target*

options. PSA the reproducer. I considered two approaches:

a) raise an ERROR when these parameter were set. check_subscriber() can do it

b) overwrite these GUCs as empty strings. 

10.

The execution always fails if users execute --dry-run just before. Because

pg_createsubscriber stops the standby anyway. Doing dry run first is quite normal

use-case, so current implementation seems not user-friendly. How should we fix?

Below bullets are my idea:

a) avoid stopping the standby in case of dry_run: seems possible.

b) accept even if the standby is stopped: seems possible.

c) start the standby at the end of run: how arguments like pg_ctl -l should be specified?