On Mon, 19 Jun 2023 16:49:05 -0700 "Tristan Partin" <tris...@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" <tris...@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 <nag...@sraoss.co.jp>
