Re: pgbnech: allow to cancel queries during benchmark - Mailing list pgsql-hackers

Hello Fabien,

Thank you for your review!

On Mon, 3 Jul 2023 20:39:23 +0200 (CEST)
Fabien COELHO <coelho@cri.ensmp.fr> wrote:

> 
> Yugo-san,
> 
> Some feedback about v1 of this patch.
> 
> Patch applies cleanly, compiles.
> 
> There are no tests, could there be one? ISTM that one could be done with a 
> "SELECT pg_sleep(...)" script??

Agreed. I will add the test.

> The global name "all_state" is quite ambiguous, what about "client_states" 
> instead? Or maybe it could be avoided, see below.
> 
> Instead of renaming "state" to "all_state" (or client_states as suggested 
> above), I'd suggest to minimize the patch by letting "state" inside the 
> main and adding a "client_states = state;" just after the allocation, or 
> another approach, see below.

Ok, I'll fix to add a global variable "client_states" and make this point to
"state" instead of changing "state" to global.
 
> Should PQfreeCancel be called on deconnections, in finishCon? I think that 
> there may be a memory leak with the current implementation??

Agreed. I'll fix.
 
> Maybe it should check that cancel is not NULL before calling PQcancel?

I think this is already checked as below, but am I missing something?

+       if (all_state[i].cancel != NULL)
+           (void) PQcancel(all_state[i].cancel, errbuf, sizeof(errbuf));

> After looking at the code, I'm very unclear whether they may be some 
> underlying race conditions, or not, depending on when the cancel is 
> triggered. I think that some race conditions are still likely given the 
> current thread 0 implementation, and dealing with them with a barrier or 
> whatever is not desirable at all.
> 
> In order to work around this issue, ISTM that we should go away from the 
> simple and straightforward thread 0 approach, and the only clean way is 
> that the cancelation should be managed by each thread for its own client.
> 
> I'd suggest to have the advanceState to call PQcancel when CancelRequested 
> is set and switch to CSTATE_ABORTED to end properly. This means that there 
> would be no need for the global client states pointer, so the patch should 
> be smaller and simpler. Possibly there would be some shortcuts added here 
> and there to avoid lingering after the control-C, in threadRun.

I am not sure this approach is simpler than mine. 

In multi-threads, only one thread can catches the signal and other threads
continue to run. Therefore, if Ctrl+C is pressed while threads are waiting
responses from the backend in wait_on_socket_set, only one thread can be
interrupted and return, but other threads will continue to wait and cannot
check CancelRequested. So, for implementing your suggestion, we need any hack
to make all threads return from wait_on_socket_set when the event occurs, but
I don't have idea to do this in simpler way. 

In my patch, all threads can return from wait_on_socket_set at Ctrl+C
because when thread #0 cancels all connections, the following error is
sent to all sessions:

 ERROR:  canceling statement due to user request

and all threads will receive the response from the backend.

Regards,
Yugo Nagata

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