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