RE: Perform streaming logical transactions by background workers and parallel apply - Mailing list pgsql-hackers

On Thu, Aug 4, 2022 2:36 PM Wang, Wei/王 威 <wangw.fnst@fujitsu.com> wrote:
> 
> I also did some other improvements based on the suggestions posted in this
> thread. Attach the new patches.
> 

Thanks for updating the patch. Here are some comments on v20-0001 patch.

1.
+typedef struct ApplyBgworkerShared
+{
+    slock_t    mutex;
+
+    /* Status of apply background worker. */
+    ApplyBgworkerStatus    status;
+
+    /* proto version of publisher. */
+    uint32    proto_version;
+
+    TransactionId    stream_xid;
+
+    /* id of apply background worker */
+    uint32    worker_id;
+} ApplyBgworkerShared;

Would it be better to modify the comment of "proto_version" to "Logical protocol
version"?

2. commnent of handle_streamed_transaction()

+ * Exception: When the main apply worker is applying streaming transactions in
+ * parallel mode (e.g. when addressing LOGICAL_REP_MSG_RELATION or
+ * LOGICAL_REP_MSG_TYPE changes), then return false.

This comment doesn't look very clear, could we change it to:

Exception: In SUBSTREAM_PARALLEL mode, if the message type is
LOGICAL_REP_MSG_RELATION or LOGICAL_REP_MSG_TYPE, return false even if this is
the main apply worker.

3.
+/*
+ * There are three fields in message: start_lsn, end_lsn and send_time. Because
+ * we have updated these statistics in apply worker, we could ignore these
+ * fields in apply background worker. (see function LogicalRepApplyLoop)
+ */
+#define SIZE_STATS_MESSAGE (3 * sizeof(uint64))

updated these statistics in apply worker
->
updated these statistics in main apply worker

4.
+static void
+LogicalApplyBgwLoop(shm_mq_handle *mqh, volatile ApplyBgworkerShared *shared)
+{
+    shm_mq_result shmq_res;
+    PGPROC       *registrant;
+    ErrorContextCallback errcallback;

I think we can define "shmq_res" in the for loop.

5.
+        /*
+         * We use first byte of message for additional communication between
+         * main Logical replication worker and apply background workers, so if
+         * it differs from 'w', then process it first.
+         */

between main Logical replication worker and apply background workers
->
between main apply worker and apply background workers

Regards,
Shi yu