Re: pgbnech: allow to cancel queries during benchmark

On Wed, 2 Aug 2023 16:37:53 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:

> Hello Fabien,
> 
> On Fri, 14 Jul 2023 20:32:01 +0900
> Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> 
> I attached the updated patch.

I'm sorry. I forgot to attach the patch.

Regards,
Yugo Nagata

> 
> > 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.
> 
> I added a TAP 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.
> 
> Done.
> 
> >  
> > > 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.
> 
> Done.
> 
> Regards,
> Yugo Nagata
> 
> >  
> > > 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>
> > 
> > 
> 
> 
> -- 
> Yugo NAGATA <nagata@sraoss.co.jp>
> 
> 


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