Re: pgbnech: allow to cancel queries during benchmark

On Mon, 15 Jan 2024 16:49:44 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

> > On Wed, 6 Sep 2023 20:13:34 +0900
> > Yugo NAGATA <nagata@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.

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

> > The commit
> > fixes this so that cancel requests are sent for all connections
> > before pgbench exits.
> 
> "sent for" -> "sent to"

Fixed.

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

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));


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

Fixed.

The attached is the updated patch, v5.

Regards,
Yugo Nagata

> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp


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