RE: Perform streaming logical transactions by background workers and parallel apply

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