Subject: Re: pgbnech: allow to cancel queries during benchmark

From: Tatsuo Ishii <[email-masked]>
Date: Mon, 15 Jan 2024 16:49:44 +0900 (JST)

> On Wed, 6 Sep 2023 20:13:34 +0900
> Yugo NAGATA <[email-masked]> 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

---

From: Yugo NAGATA <[email-masked]>
Date: Fri, 19 Jan 2024 16:51:20 +0900

On Mon, 15 Jan 2024 16:49:44 +0900 (JST)

> until they tried to send the result to pgbench.

Thank you for your comments. I agree with you, so I fixed the message
as your suggestion.

> > fixes this so that cancel requests are sent for all connections

Fixed.

> think it's better to print out the error message in case of error.

Is the message useful for pgbench users? I saw the error is ignored
in pg_dump, for example in bin/pg_dump/parallel.c

        /*
         * Send QueryCancel to leader connection, if enabled.  Ignore errors,
         * there's not much we can do about them anyway.
         */
        if (signal_info.myAH != NULL && signal_info.myAH->connCancel != NULL)
            (void) PQcancel(signal_info.myAH->connCancel,
                            errbuf, sizeof(errbuf));

> 	 * that, the thread sends cancel requests to all benchmark queries.

Fixed.

The attached is the updated patch, v5.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <[email-masked]>

---

From: Tatsuo Ishii <[email-masked]>
Date: Fri, 19 Jan 2024 17:46:03 +0900 (JST)

> in pg_dump, for example in bin/pg_dump/parallel.c

I think the situation is different from pg_dump.  Unlike pg_dump, if
PQcancel does not work, users can fix the problem by using
pg_terminate_backend or kill command. In order to make this work, an
appropriate error message is essential.

Best reagards,

---

From: Yugo NAGATA <[email-masked]>
Date: Wed, 24 Jan 2024 22:17:44 +0900

On Fri, 19 Jan 2024 17:46:03 +0900 (JST)

> appropriate error message is essential.

Makes sense. I fixed to emit an error message when PQcancel fails.

Also, I added some comments about the signal handling on Windows
to explain why the different way than non-Windows is required;

+    * On Windows, a callback function is set in which query cancel requests
+    * are sent to all benchmark queries running in the backend. This is
+    * required because all threads running queries continue to run without
+    * interrupted even when the signal is received.
+    *

Attached is the updated patch, v6.