Re: speed up a logical replica setup

01.

```

/* Options */

static char *pub_conninfo_str = NULL;

static SimpleStringList database_names = {NULL, NULL};

static int wait_seconds = DEFAULT_WAIT;

static bool retain = false;

static bool dry_run = false;

```

Just to confirm - is there a policy why we store the specified options? If you

want to store as global ones, username and port should follow (my fault...).

Or, should we have a structure to store them?

04.

```

{"dry-run", no_argument, NULL, 'n'},

```

I'm not sure why the dry_run mode exists. In terms pg_resetwal, it shows the

which value would be changed based on the input. As for the pg_upgrade, it checks

whether the node can be upgraded for now. I think, we should have the checking

feature, so it should be renamed to --check. Also, the process should exit earlier

at that time.

05.

I felt we should accept some settings from enviroment variables, like pg_upgrade.

Currently, below items should be acceted.

- data directory

- username

- port

- timeout

06.

```

pg_logging_set_level(PG_LOG_WARNING);

```

If the default log level is warning, there are no ways to output debug logs.

(-v option only raises one, so INFO would be output)

I think it should be PG_LOG_INFO.

07.

Can we combine verifications into two functions, e.g., check_primary() and check_standby/check_subscriber()?

08.

Not sure, but if we want to record outputs by pg_subscriber, the sub-directory

should be created. The name should contain the timestamp.

09.

Not sure, but should we check max_slot_wal_keep_size of primary server? It can

avoid to fail starting of logical replicaiton.

10.

```

nslots_new = nslots_old + dbarr.ndbs;

if (nslots_new > max_replication_slots)

{

pg_log_error("max_replication_slots (%d) must be greater than or equal to "

"the number of replication slots required (%d)", max_replication_slots, nslots_new);

exit(1);

}

```

I think standby server must not have replication slots. Because subsequent

pg_resetwal command discards all the WAL file, so WAL records pointed by them

are removed. Currently pg_resetwal does not raise ERROR at that time.

11.

```

/*

* Stop the subscriber if it is a standby server. Before executing the

* transformation steps, make sure the subscriber is not running because

* one of the steps is to modify some recovery parameters that require a

* restart.

*/

if (stat(pidfile, &statbuf) == 0)

```

I kept just in case, but I'm not sure it is still needed. How do you think?

Removing it can reduce an inclusion of pidfile.h.

12.

```

pg_ctl_cmd = psprintf("\"%s/pg_ctl\" stop -D \"%s\" -s",

  standby.bindir, standby.pgdata);

rc = system(pg_ctl_cmd);

pg_ctl_status(pg_ctl_cmd, rc, 0);

```

There are two places to stop the instance. Can you divide it into a function?

13.

```

* A temporary replication slot is not used here to avoid keeping a

* replication connection open (depending when base backup was taken, the

* connection should be open for a few hours).

*/

conn = connect_database(primary.base_conninfo, dbarr.perdb[0].dbname);

if (conn == NULL)

exit(1);

consistent_lsn = create_logical_replication_slot(conn, true,

&dbarr.perdb[0]);

```

I didn't notice the comment, but still not sure the reason. Why we must reserve

the slot until pg_subscriber finishes? IIUC, the slot would be never used, it

is created only for getting a consistent_lsn. So we do not have to keep.

Also, just before, logical replication slots for each databases are created, so

WAL records are surely reserved.

14.

```

pg_log_info("starting the subscriber");

start_standby_server(&standby, subport, server_start_log);

```

This info should be in the function.

15.

```

/*

* Create a subscription for each database.

*/

for (i = 0; i < dbarr.ndbs; i++)

```

This can be divided into a function, like create_all_subscriptions().

16.

My fault: usage() must be updated.

17. use_primary_slot_name

```

if (PQntuples(res) != 1)

{

pg_log_error("could not obtain replication slot information: got %d rows, expected %d row",

PQntuples(res), 1);

return NULL;

}

```

Error message should be changed. I think this error means the standby has wrong primary_slot_name, right?

18. misc

Sometimes the pid of pg_subscriber is referred. It can be stored as global variable.

19.

C99-style has been allowed, so loop variables like "i" can be declared in the for-statement, like

```

for (int i = 0; i < MAX; i++)

```

20.

Some comments, docs, and outputs must be fixed when the name is changed.