Re: speed up a logical replica setup

On Wed, 7 Feb 2024 at 10:24, Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Fri, Feb 2, 2024, at 6:41 AM, Hayato Kuroda (Fujitsu) wrote:
>
> Thanks for updating the patch!

Thanks for the updated patch, few comments:
Few comments:
1) Cleanup function handler flag should be reset, i.e.
dbinfo->made_replslot = false; should be there else there will be an
error during drop replication slot cleanup in error flow:
+static void
+drop_replication_slot(PGconn *conn, LogicalRepInfo *dbinfo, const
char *slot_name)
+{
+ PQExpBuffer str = createPQExpBuffer();
+ PGresult *res;
+
+ Assert(conn != NULL);
+
+ pg_log_info("dropping the replication slot \"%s\" on database
\"%s\"", slot_name, dbinfo->dbname);
+
+ appendPQExpBuffer(str, "SELECT
pg_drop_replication_slot('%s')", slot_name);
+
+ pg_log_debug("command is: %s", str->data);

2) Cleanup function handler flag should be reset, i.e.
dbinfo->made_publication = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove publication if it couldn't finish all steps.
+ */
+static void
+drop_publication(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+ PQExpBuffer str = createPQExpBuffer();
+ PGresult *res;
+
+ Assert(conn != NULL);
+
+ pg_log_info("dropping publication \"%s\" on database \"%s\"",
dbinfo->pubname, dbinfo->dbname);
+
+ appendPQExpBuffer(str, "DROP PUBLICATION %s", dbinfo->pubname);
+
+ pg_log_debug("command is: %s", str->data);

3) Cleanup function handler flag should be reset, i.e.
dbinfo->made_subscription = false; should be there else there will be
an error during drop publication cleanup in error flow:
+/*
+ * Remove subscription if it couldn't finish all steps.
+ */
+static void
+drop_subscription(PGconn *conn, LogicalRepInfo *dbinfo)
+{
+ PQExpBuffer str = createPQExpBuffer();
+ PGresult *res;
+
+ Assert(conn != NULL);
+
+ pg_log_info("dropping subscription \"%s\" on database \"%s\"",
dbinfo->subname, dbinfo->dbname);
+
+ appendPQExpBuffer(str, "DROP SUBSCRIPTION %s", dbinfo->subname);
+
+ pg_log_debug("command is: %s", str->data);

4) I was not sure if drop_publication is required here, as we will not
create any publication in subscriber node:
+ if (dbinfo[i].made_subscription)
+ {
+ conn = connect_database(dbinfo[i].subconninfo);
+ if (conn != NULL)
+ {
+ drop_subscription(conn, &dbinfo[i]);
+ if (dbinfo[i].made_publication)
+ drop_publication(conn, &dbinfo[i]);
+ disconnect_database(conn);
+ }
+ }

5) The connection should be disconnected in case of error case:
+ /* 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));
+ return NULL;
+ }
+ PQclear(res);

6) There should be a line break before postgres_fe inclusion, to keep
it consistent:
+ *-------------------------------------------------------------------------
+ */
+#include "postgres_fe.h"
+
+#include <signal.h>

7) These includes are not required:
7.a) #include <signal.h>
7.b) #include <sys/stat.h>
7.c) #include <sys/wait.h>
7.d) #include "access/xlogdefs.h"
7.e) #include "catalog/pg_control.h"
7.f) #include "common/file_utils.h"
7.g) #include "utils/pidfile.h"

+ * src/bin/pg_basebackup/pg_createsubscriber.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres_fe.h"
+
+#include <signal.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/wait.h>
+#include <time.h>
+
+#include "access/xlogdefs.h"
+#include "catalog/pg_authid_d.h"
+#include "catalog/pg_control.h"
+#include "common/connect.h"
+#include "common/controldata_utils.h"
+#include "common/file_perm.h"
+#include "common/file_utils.h"
+#include "common/logging.h"
+#include "common/restricted_token.h"
+#include "fe_utils/recovery_gen.h"
+#include "fe_utils/simple_list.h"
+#include "getopt_long.h"
+#include "utils/pidfile.h"

8) POSTMASTER_STANDBY and POSTMASTER_FAILED are not being used, is it
required or kept for future purpose:
+enum WaitPMResult
+{
+ POSTMASTER_READY,
+ POSTMASTER_STANDBY,
+ POSTMASTER_STILL_STARTING,
+ POSTMASTER_FAILED
+};

9) pg_createsubscriber should be kept after pg_basebackup to maintain
the consistent order:
diff --git a/src/bin/pg_basebackup/.gitignore b/src/bin/pg_basebackup/.gitignore
index 26048bdbd8..b3a6f5a2fe 100644
--- a/src/bin/pg_basebackup/.gitignore
+++ b/src/bin/pg_basebackup/.gitignore
@@ -1,5 +1,6 @@
/pg_basebackup
/pg_receivewal
/pg_recvlogical
+/pg_createsubscriber

10) dry-run help message is not very clear, how about something
similar to pg_upgrade's message like "check clusters only, don't
change any data":
+ printf(_(" -d, --database=DBNAME database to
create a subscription\n"));
+ printf(_(" -n, --dry-run stop before
modifying anything\n"));
+ printf(_(" -t, --recovery-timeout=SECS seconds to wait
for recovery to end\n"));
+ printf(_(" -r, --retain retain log file
after success\n"));
+ printf(_(" -v, --verbose output verbose
messages\n"));
+ printf(_(" -V, --version output version
information, then exit\n"));

Regards,
Vignesh