Re: speed up a logical replica setup

On Thu, 7 Mar 2024 at 10:05, Euler Taveira <euler@eulerto.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