On Thursday, September 22, 2022 4:08 PM Kuroda, Hayato/黒田 隼人 <kuroda.hayato@fujitsu.com> 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
