Hello Fabien, On Mon, 7 Aug 2023 12:17:38 +0200 (CEST) Fabien COELHO <coe...@cri.ensmp.fr> wrote:
> > Hello Yugo-san, > > > I attached v2 patch including the documentation and some comments > > in the code. > > I've looked at this patch. Thank you for your review! > > I'm unclear whether it does what it says: "exit immediately on abort", I > would expect a cold call to "exit" (with a clear message obviously) when > the abort occurs. > > Currently it skips to "done" which starts by closing this particular > thread client connections, then it will call "exit" later, so it is not > "immediate". There are cases where "goto done" is used where some error like "invalid socket: ..." happens. I would like to make pgbench exit in such cases, too, so I chose to handle errors below the "done:" label. However, we can change it to call "exit" instead of "goo done" at each place. Do you think this is better? > I do not think that this cleanup is necessary, because anyway all other > threads will be brutally killed by the exit called by the aborted thread, > so why bothering to disconnect only some connections? Agreed. This disconnection is not necessary anyway even when we would like to handle it below "done". > /* If any client is aborted, exit immediately. */ > if (state[i].state != CSTATE_FINISHED) > > For this comment, I would prefer "if ( ... == CSTATE_ABORTED)" rather that > implying that not finished means aborted, and if you follow my previous > remark then this code can be removed. Ok. If we handle errors like "invalid socket:" (mentioned above) after skipping to "done", we should set the status to CSTATE_ABORTED before the jump. Otherwise, if we choose to call "exit" immediately at each error instead of skipping to "done", we can remove this as you says. > Typo: "going to exist" -> "going to exit". Note that it is not "the whole > thread" but "the program" which is exiting. I'll fix. > There is no test. I'll add an test. Regards, Yugo Nagata > -- > Fabien. -- Yugo NAGATA <nag...@sraoss.co.jp>
