Re: Parallel Apply

Some review comments for v17-0001.

======
Commit Message

1.
Patch also adds LOGICAL_REP_MSG_INTERNAL_MESSAGE, so we need to
describe how that works and how it has a double-meaning.

======
.../replication/logical/applyparallelworker.c

apply_handle_internal_relation:

2.
+ int nrels;
+
+ nrels = pq_getmsgint(s, 4);

Should assign value at the declaration so it is consistent with
apply_handle_internal_dependency.

~~~

LogicalParallelApplyLoop:

3.
We also need to statically assert that
LOGICAL_REP_MSG_INTERNAL_MESSAGE cannot clash with PqReplMsg_WALData
and a comment to explain why that assert is necessary (e.g. because of
the double-meaning of LOGICAL_REP_MSG_INTERNAL_MESSAGE).
LogicalParallelApplyLoop might be a good place to put this assert.

~~~

pa_wait_for_depended_transaction:

4.
+/*
+ * Wait for the given transaction to finish.
+ *
+ * Both leader and parallel apply workers can call this function to wait for a
+ * transaction to finish.
+ */
+void
+pa_wait_for_depended_transaction(TransactionId xid)
+{
+ elog(DEBUG1, "wait for depended xid %u", xid);
+
+ for (;;)
+ {
+ /* XXX wait until given transaction is finished */
+ }
+
+ elog(DEBUG1, "finish waiting for depended xid %u", xid);
+}

I don't think we should code infinite CPU loops, even if they are
intended to be only temporary. IMO each patch needs to be valid
standalone. So, it's better to use some "safer" code here -- e.g.
maybe something that iterates a sleep 5s and will give RTE if it waits
more than 5 min. Also, add a comment to say it is temporary code to
be replaced by the next patch in this set.

======
src/include/replication/worker_internal.h

5.
+/*
+ * Parallel Apply worker internal message types
+ *
+ * This type of messages would be generated by leader apply worker and sent to
+ * the parallel apply worker.
+ */
+typedef enum PAWorkerMsgType
+{
+ PA_MSG_XACT_DEPENDENCY = 'd',
+ PA_MSG_RELMAP = 'r',
+} PAWorkerMsgType;
+

5a.
typo -- /This type of messages would be/These types of messages are/
typo -- /leader apply worker/the leader apply worker/

~

5b.
Comment should also say that a PAWorkerMsgType within a message will
always be introduced by the LOGICAL_REP_MSG_INTERNAL_MESSAGE byte.

======
Kind Regards,
Peter Smith.
Fujitsu Australia