> On Wed, 6 Sep 2023 20:13:34 +0900 > Yugo NAGATA <nag...@sraoss.co.jp> wrote: > >> I attached the updated patch v3. The changes since the previous >> patch includes the following; >> >> I removed the unnecessary condition (&& false) that you >> pointed out in [1]. >> >> The test was rewritten by using IPC::Run signal() and integrated >> to "001_pgbench_with_server.pl". This test is skipped on Windows >> because SIGINT causes to terminate the test itself as discussed >> in [2] about query cancellation test in psql. >> >> I added some comments to describe how query cancellation is >> handled as I explained in [1]. >> >> Also, I found the previous patch didn't work on Windows so fixed it. >> On non-Windows system, a thread waiting a response of long query can >> be interrupted by SIGINT, but on Windows, threads do not return from >> waiting until queries they are running are cancelled. This is because, >> when the signal is received, the system just creates a new thread to >> execute the callback function specified by setup_cancel_handler, and >> other thread continue to run[3]. Therefore, the queries have to be >> cancelled in the callback function. >> >> [1] >> https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr >> [2] >> https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp >> [3] https://learn.microsoft.com/en-us/windows/console/handlerroutine > > I found that --disable-thread-safety option was removed in 68a4b58eca0329. > So, I removed codes involving ENABLE_THREAD_SAFETY from the patch. > > Also, I wrote a commit log draft.
> Previously, Ctrl+C during benchmark killed pgbench immediately, > but queries running at that time were not cancelled. Better to mention explicitely that queries keep on running on the backend. What about this? Previously, Ctrl+C during benchmark killed pgbench immediately, but queries were not canceled and they keep on running on the backend until they tried to send the result to pgbench. > The commit > fixes this so that cancel requests are sent for all connections > before pgbench exits. "sent for" -> "sent to" > Attached is the updated version, v4. +/* send cancel requests to all connections */ +static void +cancel_all() +{ + for (int i = 0; i < nclients; i++) + { + char errbuf[1]; + if (client_states[i].cancel != NULL) + (void) PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf)); + } +} + Why in case of errors from PQCancel the error message is neglected? I think it's better to print out the error message in case of error. + * On non-Windows, any callback function is not set. When SIGINT is + * received, CancelRequested is just set, and only thread #0 is interrupted + * and returns from waiting input from the backend. After that, the thread + * sends cancel requests to all benchmark queries. The second line is a little bit long according to the coding standard. Fix like this? * On non-Windows, any callback function is not set. When SIGINT is * received, CancelRequested is just set, and only thread #0 is * interrupted and returns from waiting input from the backend. After * that, the thread sends cancel requests to all benchmark queries. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp