Re: Make pgbench exit on SIGINT more reliably - Mailing list pgsql-hackers

On Mon Jun 19, 2023 at 6:39 AM PDT, Yugo NAGATA wrote:
> On Wed, 24 May 2023 08:58:46 -0500
> "Tristan Partin" <tristan@neon.tech> wrote:
>
> > On Tue May 23, 2023 at 7:31 PM CDT, Michael Paquier wrote:
> > > On Mon, May 22, 2023 at 10:02:02AM -0500, Tristan Partin wrote:
> > > > The way that pgbench handled SIGINT changed in
> > > > 1d468b9ad81b9139b4a0b16b416c3597925af4b0. Unfortunately this had a
> > > > couple of unintended consequences, at least from what I can tell[1].
> > > >
> > > > - CTRL-C no longer stops the program unless the right point in pgbench
> > > >   execution is hit
> > > > - pgbench no longer exits with a non-zero exit code
> > > >
> > > > An easy reproduction of these problems is to run with a large scale
> > > > factor like: pgbench -i -s 500000. Then try to CTRL-C the program.
> > >
> > > This comes from the code path where the data is generated client-side,
> > > and where the current CancelRequested may not be that responsive,
> > > isn't it?
> >
> > It comes from the fact that CancelRequested is only checked within the
> > generation of the pgbench_accounts table, but with a large enough scale
> > factor or enough latency, say cross-continent communication, generating
> > the other tables can be time consuming too. But, yes you are more likely
> > to run into this issue when generating client-side data.
>
> If I understand correctly, your patch allows to exit pgbench immediately
> during a client-side data generation even while the tables other than
> pgbench_accounts are processed. It can be useful when the scale factor
> is large. However, the same purpose seems to be achieved by you other patch [1].
> Is the latter your latest proposal that replaces the patch in this thread?ad?
>
> [1] https://www.postgresql.org/message-id/flat/CSTU5P82ONZ1.19XFUGHMXHBRY%40c3po

The other patch does not replace this one. They are entirely separate.

> Also, your proposal includes to change the exit code when pgbench is cancelled by
> SIGINT. I think it is nice because this will make it easy to understand and clarify
> the result of the initialization.
>
> Currently, pgbench returns 0 when the initialization is cancelled while populating
> pgbench_branches or pgbench_tellers, but the resultant pgbench_accounts has only
> one row, which is an inconsistent result. Returning the specific value for SIGINT
> cancel can let user know it explicitly. In addition, it seems better if an error
> message to inform would be output.
>
> For the above purpose, the patch moved exit codes of psql to fe_utils to be shared.
> However,  I am not sure this is good way. Each frontend-tool may have it own exit code,
> for example. "2" means "bad connection" in psql [2], but "errors during the run" in
> pgbench [3]. So, I think it is better to define them separately.
>
> [2] https://www.postgresql.org/docs/current/app-psql.html#id-1.9.4.20.7
> [3] https://www.postgresql.org/docs/current/pgbench.html#id=id-1.9.4.11.7

I can change it. I just figured that sharing exit code definitions
would've been preferrable. I will post a v3 some time soon which removes
that patch.

Thanks for your review :).

--
Tristan Partin
Neon (https://neon.tech)