Re: speed up a logical replica setup

On Fri, 22 Mar 2024 at 09:01, Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Thu, Mar 21, 2024, at 6:49 AM, Shlok Kyal wrote:
>
> There is a compilation error while building postgres with the patch
> due to a recent commit. I have attached a top-up patch v32-0003 to
> resolve this compilation error.
> I have not updated the version of the patch as I have not made any
> change in v32-0001 and v32-0002 patch.
>
>
> I'm attaching a new version (v33) to incorporate this fix (v32-0003) into the
> main patch (v32-0001). This version also includes 2 new tests:
>
> - refuse to run if the standby server is running
> - refuse to run if the standby was promoted e.g. it is not in recovery
>
> The first one exercises a recent change (standby should be stopped) and the
> second one covers an important requirement.

Few comments:
1) In error case PQclear and PQfinish should be called:
+ /* Secure search_path */
+ res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("could not clear search_path: %s",
+ PQresultErrorMessage(res));
+ if (exit_on_error)
+ exit(1);
+
+ return NULL;
+ }
+ PQclear(res);

2) Call fflush here before calling system command to get proper
ordered console output:
a) Call fflush:
+ int rc = system(cmd_str);
+
+ if (rc == 0)
+ pg_log_info("subscriber successfully changed
the system identifier");
+ else
+ pg_fatal("subscriber failed to change system
identifier: exit code: %d", rc);
+ }

b) similarly here:
+ pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd->data);
+ rc = system(pg_ctl_cmd->data);
+ pg_ctl_status(pg_ctl_cmd->data, rc);
+ standby_running = true;

c) similarly here:
+ pg_ctl_cmd = psprintf("\"%s\" stop -D \"%s\" -s", pg_ctl_path,
+ datadir);
+ pg_log_debug("pg_ctl command is: %s", pg_ctl_cmd);
+ rc = system(pg_ctl_cmd);
+ pg_ctl_status(pg_ctl_cmd, rc);
+ standby_running = false;

3) Call PQClear in error cases:
a) Call PQClear
+ res = PQexec(conn, "SELECT system_identifier FROM
pg_catalog.pg_control_system()");
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("could not get system identifier: %s",
+ PQresultErrorMessage(res));
+ disconnect_database(conn, true);
+ }

b) similarly here
+ if (PQntuples(res) != 1)
+ {
+ pg_log_error("could not get system identifier: got %d
rows, expected %d row",
+ PQntuples(res), 1);
+ disconnect_database(conn, true);
+ }
+

c) similarly here
+ res = PQexec(conn,
+ "SELECT oid FROM pg_catalog.pg_database "
+ "WHERE datname =
pg_catalog.current_database()");
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("could not obtain database OID: %s",
+ PQresultErrorMessage(res));
+ disconnect_database(conn, true);
+ }
+

d) There are few more places like this.

4) Since we are using a global variable, we might be able to remove
initializing many of them.
+ /* Default settings */
+ subscriber_dir = NULL;
+ opt.config_file = NULL;
+ opt.pub_conninfo_str = NULL;
+ opt.socket_dir = NULL;
+ opt.sub_port = DEFAULT_SUB_PORT;
+ opt.sub_username = NULL;
+ opt.database_names = (SimpleStringList){0};
+ opt.recovery_timeout = 0;

Regards,
Vignesh