public inbox for [email protected]
help / color / mirror / Atom feedFrom: [email protected] <[email protected]>
To: [email protected] <[email protected]>
To: [email protected] <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
Date: Thu, 22 Sep 2022 08:57:24 +0000
Message-ID: <OS0PR01MB571644369FE905F1A9C92B35944E9@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <TYAPR01MB5866727DCFC8B47FAC46ABF6F54E9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: <OS0PR01MB5716730C06159452335D870D947F9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
<CAA4eK1Lu-6oXMk7ZaGYLwm3CRLBuzueGbasyHnNpJxu6Mq3mmg@mail.gmail.com>
<OS3PR01MB6275F145878B4A44586C46CE9E499@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAA4eK1LMVdS6uM7Tw7ANL0BetAd76TKkmAXNNQa0haTe2tax6g@mail.gmail.com>
<OS3PR01MB6275EFC4B707650DAB9392859E4D9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<CAHut+PuVjRgGr4saN7qwq0oB8DANHVR7UfDiciB1Q3cYN54F6A@mail.gmail.com>
<OS3PR01MB627567CF4B96A23DAFAF77269E4E9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
<TYAPR01MB5866727DCFC8B47FAC46ABF6F54E9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
On Thursday, September 22, 2022 4:08 PM Kuroda, Hayato/黒田 隼人 <[email protected]> wrote:
>
> Thanks for updating the patch! Followings are comments for v33-0001.
Thanks for the comments.
> 04. HandleParallelApplyMessages()
>
> ```
> + if (winfo->error_mq_handle == NULL)
> + continue;
> ```
>
> a.
> I was not sure when the cell should be cleaned. Currently we clean up
> ParallelApplyWorkersList() only in the parallel_apply_start_worker(), but we
> have chances to remove such a cell like HandleParallelApplyMessages() or
> HandleParallelApplyMessage(). How do you think?
HandleParallelApplyxx functions are signal callback functions which I think
are unsafe to cleanup the list cells that may be in use before entering
these signal callback functions.
>
> 05. parallel_apply_setup_worker()
>
> ``
> + if (launched)
> + {
> + ParallelApplyWorkersList = lappend(ParallelApplyWorkersList,
> winfo);
> + }
> ```
>
> {} should be removed.
I think this style is fine and this was also suggested to be consistent with the
else{} part.
>
> 06. parallel_apply_wait_for_xact_finish()
>
> ```
> + /* If any workers have died, we have failed. */
> ```
>
> This function checked only about a parallel apply worker, so the comment
> should be "if worker has..."?
The comments seem clear to me as it's a general comment.
Best regards,
Hou zj
view thread (625+ 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], [email protected]
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
In-Reply-To: <OS0PR01MB571644369FE905F1A9C92B35944E9@OS0PR01MB5716.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