public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Peter Smith <[email protected]>
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
Date: Wed, 27 Jul 2022 12:41:43 +0000
Message-ID: <TYAPR01MB58663639635E362A074DABF9F5979@TYAPR01MB5866.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OS3PR01MB62752B29CEC2592E18236A9A9E909@OS3PR01MB6275.jpnprd01.prod.outlook.com>
References: <OS3PR01MB62758DBE8FA12BA72A43AC819EB89@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+Pu5ah+NhYMfb7KTbkYTxnuGY2j-7vT0G1MZZsS4SKiHew@mail.gmail.com>
<OS3PR01MB62755C6C9A75EB09F7218B589E839@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<OS3PR01MB627594D75E870BBB45E2E80A9E839@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+PvN7fwtUE=bidzrsOUXSt+JpnkJztZ-Jn5t86moofaZ6g@mail.gmail.com>
<OS3PR01MB62758A6AAED27B3A848CEB7A9E8F9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<OS3PR01MB62752B29CEC2592E18236A9A9E909@OS3PR01MB6275.jpnprd01.prod.outlook.com>
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
view thread (633+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
In-Reply-To: <TYAPR01MB58663639635E362A074DABF9F5979@TYAPR01MB5866.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox