Hello Fabien,
On Mon, 7 Aug 2023 12:17:38 +0200 (CEST)
Fabien COELHO <coelho@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 <nagata@sraoss.co.jp>