Re: Make pgbench exit on SIGINT more reliably

On Mon, 19 Jun 2023 16:49:05 -0700
"Tristan Partin" <tristan@neon.tech> wrote:

> 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.

After applying the other patch, CancelRequested would be checked enough frequently
even during initialization of pgbench_branches and pgbench_tellers, and it will
allow the initialization step to be cancelled immediately even if the scale factor
is high. So, is it unnecessary to modify setup_cancel_handler anyway?

I think it would be still nice to introduce a new exit code for query cancel separately.

> 
> > 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.

Great!

> 
> Thanks for your review :).
> 
> -- 
> Tristan Partin
> Neon (https://neon.tech)


-- 
Yugo NAGATA <nagata@sraoss.co.jp>