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

On Fri, 22 Mar 2024 at 09:01, Euler Taveira <euler@eulerto.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