Re: speed up a logical replica setup

On Thu, 7 Mar 2024 at 10:05, Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Wed, Mar 6, 2024, at 7:02 AM, Hayato Kuroda (Fujitsu) wrote:
>
> Thanks for updating the patch!
>
>
> Thanks for the feedback. I'm attaching v26 that addresses most of your comments
> and some issues pointed by Vignesh [1].

Few comments:
1) We are disconnecting database again in error case too, it will lead
to a double free in this scenario,
+ PQclear(res);
+
+ disconnect_database(conn, false);
+
+ if (max_repslots < num_dbs)
+ {
+ pg_log_error("subscriber requires %d replication
slots, but only %d remain",
+ num_dbs, max_repslots);
+ pg_log_error_hint("Consider increasing
max_replication_slots to at least %d.",
+ num_dbs);
+ disconnect_database(conn, true);
+ }
+
+ if (max_lrworkers < num_dbs)
+ {
+ pg_log_error("subscriber requires %d logical
replication workers, but only %d remain",
+ num_dbs, max_lrworkers);
+ pg_log_error_hint("Consider increasing
max_logical_replication_workers to at least %d.",
+ num_dbs);
+ disconnect_database(conn, true);
+ }

pg_createsubscriber: error: subscriber requires 5 logical replication
workers, but only 4 remain
pg_createsubscriber: hint: Consider increasing
max_logical_replication_workers to at least 5.
free(): double free detected in tcache 2
Aborted

2) We can also check that the primary is not using
synchronous_standby_names, else all the transactions in the primary
will wait infinitely once the standby server is stopped, this could be
added in the documentation:
+/*
+ * Is the primary server ready for logical replication?
+ */
+static void
+check_publisher(struct LogicalRepInfo *dbinfo)
+{
+ PGconn *conn;
+ PGresult *res;
+
+ char *wal_level;
+ int max_repslots;
+ int cur_repslots;
+ int max_walsenders;
+ int cur_walsenders;
+
+ pg_log_info("checking settings on publisher");
+
+ conn = connect_database(dbinfo[0].pubconninfo, true);
+
+ /*
+ * If the primary server is in recovery (i.e. cascading replication),
+ * objects (publication) cannot be created because it is read only.
+ */
+ if (server_is_in_recovery(conn))
+ {
+ pg_log_error("primary server cannot be in recovery");
+ disconnect_database(conn, true);
+ }

3) This check is present only for publication, we do not have this in
case of creating a subscription. We can keep both of them similar,
i.e. have the check in both or don't have the check for both
publication and subscription:
+ /* Check if the publication already exists */
+ appendPQExpBuffer(str,
+ "SELECT 1 FROM
pg_catalog.pg_publication "
+ "WHERE pubname = '%s'",
+ dbinfo->pubname);
+ res = PQexec(conn, str->data);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("could not obtain publication information: %s",
+ PQresultErrorMessage(res));
+ disconnect_database(conn, true);
+ }
+

4) Few options are missing:
+ <refsynopsisdiv>
+ <cmdsynopsis>
+ <command>pg_createsubscriber</command>
+ <arg rep="repeat"><replaceable>option</replaceable></arg>
+ <group choice="plain">
+ <group choice="req">
+ <arg choice="plain"><option>-d</option></arg>
+ <arg choice="plain"><option>--database</option></arg>
+ </group>
+ <replaceable>dbname</replaceable>
+ <group choice="req">
+ <arg choice="plain"><option>-D</option> </arg>
+ <arg choice="plain"><option>--pgdata</option></arg>
+ </group>
+ <replaceable>datadir</replaceable>
+ <group choice="req">
+ <arg choice="plain"><option>-P</option></arg>
+ <arg choice="plain"><option>--publisher-server</option></arg>
+ </group>
+ <replaceable>connstr</replaceable>
+ </group>
+ </cmdsynopsis>
+ </refsynopsisdiv>

4a) -n, --dry-run
4b) -p, --subscriber-port
4c) -r, --retain
4d) -s, --socket-directory
4e) -t, --recovery-timeout
4f) -U, --subscriber-username

5) typo connnections should be connections
+ <para>
+ The port number on which the target server is listening for
connections.
+ Defaults to running the target server on port 50432 to avoid unintended
+ client connnections.

6) repliation should be replication
+ * Create the subscriptions, adjust the initial location for logical
+ * replication and enable the subscriptions. That's the last step for logical
+ * repliation setup.

7) I did not notice these changes in the latest patch:
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d808aad8b0..08de2bf4e6 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -517,6 +517,7 @@ CreateSeqStmt
CreateStatsStmt
CreateStmt
CreateStmtContext
+CreateSubscriberOptions
CreateSubscriptionStmt
CreateTableAsStmt
CreateTableSpaceStmt
@@ -1505,6 +1506,7 @@ LogicalRepBeginData
LogicalRepCommitData
LogicalRepCommitPreparedTxnData
LogicalRepCtxStruct
+LogicalRepInfo
LogicalRepMsgType
LogicalRepPartMapEntry
LogicalRepPreparedTxnData

Regards,
Vignesh