Dear Wang-san,
Hi, I'm also interested in the patch and I started to review this.
Followings are comments about 0001.
1. terminology
In your patch a new worker "apply background worker" has been introduced,
but I thought it might be confused because PostgreSQL has already the worker
"background worker".
Both of apply worker and apply bworker are categolized as bgworker.
Do you have any reasons not to use "apply parallel worker" or "apply streaming
worker"?
(Note that I'm not native English speaker)
2. logicalrep_worker_stop()
```
- /* No worker, nothing to do. */
- if (!worker)
- {
- LWLockRelease(LogicalRepWorkerLock);
- return;
- }
+ if (worker)
+ logicalrep_worker_stop_internal(worker);
+
+ LWLockRelease(LogicalRepWorkerLock);
+}
```
I thought you could add a comment the meaning of if-statement, like "No main
apply worker, nothing to do"
3. logicalrep_workers_find()
I thought you could add a description about difference between this and
logicalrep_worker_find() at the top of the function.
IIUC logicalrep_workers_find() counts subworker, but logicalrep_worker_find()
does not focus such type of workers.
4. logicalrep_worker_detach()
```
static void
logicalrep_worker_detach(void)
{
+ /*
+ * If we are the main apply worker, stop all the apply background
workers
+ * we started before.
+ *
```
I thought "we are" should be "This is", based on other comments.
5. applybgworker.c
```
+/* Apply background workers hash table (initialized on first use) */
+static HTAB *ApplyWorkersHash = NULL;
+static List *ApplyWorkersFreeList = NIL;
+static List *ApplyWorkersList = NIL;
```
I thought they should be ApplyBgWorkersXXX, because they stores information
only related with apply bgworkers.
6. ApplyBgworkerShared
```
+ TransactionId stream_xid;
+ uint32 n; /* id of apply background worker */
+} ApplyBgworkerShared;
```
I thought the field "n" is too general, how about "proc_id" or "worker_id"?
7. apply_bgworker_wait_for()
```
+ /* If any workers (or the postmaster) have died, we have
failed. */
+ if (status == APPLY_BGWORKER_EXIT)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("background worker %u failed to
apply transaction %u",
+ wstate->shared->n,
wstate->shared->stream_xid)))
```
7.a
I thought we should not mention about PM death here, because in this case
apply worker will exit at WaitLatch().
7.b
The error message should be "apply background worker %u...".
8. apply_bgworker_check_status()
```
+ errmsg("background worker %u exited
unexpectedly",
+ wstate->shared->n)));
```
The error message should be "apply background worker %u...".
9. apply_bgworker_send_data()
```
+ if (result != SHM_MQ_SUCCESS)
+ ereport(ERROR,
+
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("could not send tuples to shared-memory
queue")));
```
I thought the error message should be "could not send data to..."
because sent data might not be tuples. For example, in case of STEAM PREPARE, I
thit does not contain tuple.
10. wait_event.h
```
WAIT_EVENT_HASH_GROW_BUCKETS_REINSERT,
+ WAIT_EVENT_LOGICAL_APPLY_WORKER_STATE_CHANGE,
WAIT_EVENT_LOGICAL_SYNC_DATA,
```
I thought the event should be WAIT_EVENT_LOGICAL_APPLY_BG_WORKER_STATE_CHANGE,
because this is used when apply worker waits until the status of bgworker
changes.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
