Re: Perform streaming logical transactions by background workers and parallel apply
Amit Kapila <amit.kapila16@gmail.com>
On Fri, Oct 21, 2022 at 3:02 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.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.
Commits
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Fix invalid memory access during the shutdown of the parallel apply worker.
- 3d144c6c8602 16.0 landed
-
Fix assertion failure in apply worker.
- de63f8dadee4 16.0 landed
-
Use elog to report unexpected action in handle_streamed_transaction().
- 781ac42d43ab 16.0 landed
-
Use appropriate wait event when sending data in the apply worker.
- d9d7fe68d35e 16.0 landed
-
Allow the logical_replication_mode to be used on the subscriber.
- 9f2213a7c575 16.0 landed
-
Rename GUC logical_decoding_mode to logical_replication_mode.
- 1e8b61735cfb 16.0 landed
-
Display the leader apply worker's PID for parallel apply workers.
- d540a02a724b 16.0 landed
-
Improve the code to decide and process the apply action.
- c981d9145dea 16.0 landed
-
Document the newly added wait events added by commit 216a784829.
- cd06ccd78fcf 16.0 landed
-
Perform apply of large transactions by parallel workers.
- 216a784829c2 16.0 landed
-
Wake up a subscription's replication worker processes after DDL.
- c6e1f62e2cee 16.0 cited
-
Add copyright notices to meson files
- 8284cf5f746f 16.0 cited
-
Better document logical replication parameters
- a8500750ca0a 16.0 cited
-
Add a common function to generate the origin name.
- 776e1c8a5d14 16.0 cited
-
Harmonize parameter names in storage and AM code.
- bfcf1b34805f 16.0 cited
-
Avoid using list_length() to test for empty list.
- efd0c16becbf 16.0 cited
-
Improve two comments related to a boolean DefElem's value
- 8445f5a21d40 16.0 cited
-
Fix partition table's REPLICA IDENTITY checking on the subscriber.
- 26b3455afaa9 15.0 cited
-
Fix data inconsistency between publisher and subscriber.
- b7658c24c7c1 15.0 cited
-
Fix cache look-up failures while applying changes in logical replication.
- 5a97b1325453 15.0 cited