On Mon, Aug 8, 2022 at 10:18 AM Dilip Kumar <dilipbal...@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
