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

On Fri, Oct 21, 2022 at 3:02 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>

Few comments on the 0001 and 0003 patches:

v40-0001*
==========
1.
+ /*
+ * The queue used to transfer messages from the parallel apply worker to
+ * the leader apply worker.
+ */
+ shm_mq_handle *error_mq_handle;

Shall we say error messages instead of messages?

2.
+/*
+ * Is there a message pending in parallel apply worker which we need to
+ * receive?
+ */
+volatile sig_atomic_t ParallelApplyMessagePending = false;

Can we slightly change above comment to: "Is there a message sent by
parallel apply worker which we need to receive?"

3.
+
+ ThrowErrorData(&edata);
+
+ /* Should not reach here after rethrowing an error. */
+ error_context_stack = save_error_context_stack;

Should we instead do Assert(false) after ThrowErrorData?

4.
+ * apply worker (c) necessary information to be shared among parallel apply
+ * workers and leader apply worker (i.e. in_parallel_apply_xact flag and the
+ * corresponding LogicalRepWorker slot information).

I don't think here the comment needs to exactly say which variables
are shared. necessary information to synchronize between parallel
apply workers and leader apply worker.

5.
+ * The dynamic shared memory segment will contain (a) a shm_mq that can be
+ * used to send changes in the transaction from leader apply worker to parallel
+ * apply worker (b) another shm_mq that can be used to send errors

In both (a) and (b), instead of "can be", we can use "is".

6.
Note that we cannot skip the streaming transactions when using
+ * parallel apply workers because we cannot get the finish LSN before
+ * applying the changes.

This comment is unclear about the action of parallel apply worker when
finish LSN is set. We can add something like: "So, we don't start
parallel apply worker when finish LSN is set by the user."

v40-0003
==========
7. The function RelationGetUniqueKeyBitmap() should be defined in
relcache.c next to RelationGetIdentityKeyBitmap().

8.
+RelationGetUniqueKeyBitmap(Relation rel)
{
...
+ if (!rel->rd_rel->relhasindex)
+ return NULL;

It would be better to use "if
(!RelationGetForm(relation)->relhasindex)" so as to be consistent with
similar usage in RelationGetUniqueKeyBitmap.

9. In RelationGetUniqueKeyBitmap(), we must assert here that the
historic snapshot is set as we are not taking a lock on index rels.
The same is already ensured in RelationGetIdentityKeyBitmap(), is
there a reason to be different here?

--
With Regards,
Amit Kapila.