Re: Perform streaming logical transactions by background workers and parallel apply

On Mon, Aug 8, 2022 at 10:18 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> > Based on above, we plan to first introduce the patch to perform streaming
> > logical transactions by background workers, and then introduce parallel apply
> > normal transaction which design is different and need some additional handling.
>
> Yeah I think that makes sense.  Since the streamed transactions are
> sent to standby interleaved so we can take advantage of parallelism
> and along with that we can also avoid the I/O so that will also
> speedup.

Some review comments on the latest version of the patch.

1.
+/* Queue size of DSM, 16 MB for now. */
+#define DSM_QUEUE_SIZE    160000000

Why don't we directly use 16 *1024 * 1024, that would be exactly 16 MB
so it will match with comments and also it would be more readable.

2.
+/*
+ * 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))

Instead of assuming you have 3 uint64 why don't directly add 2 *
sizeof(XLogRecPtr) + sizeof(TimestampTz) so that if this data type
ever changes
we don't need to track that we will have to change this as well.

3.
+/*
+ * Entry for a hash table we use to map from xid to our apply background worker
+ * state.
+ */
+typedef struct ApplyBgworkerEntry
+{
+    TransactionId xid;
+    ApplyBgworkerState *wstate;
+} ApplyBgworkerEntry;

Mention in the comment of the structure or for the member that xid is
the key of the hash.  Refer to other such structures for the
reference.

I am doing a more detailed review but this is what I got so far.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com