Re: pgbnech: allow to cancel queries during benchmark

Hello Fabien,

On Fri, 14 Jul 2023 20:32:01 +0900
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> wrote:

I attached the updated patch.

> Hello Fabien,
>
> Thank you for your review!
>
> On Mon, 3 Jul 2023 20:39:23 +0200 (CEST)
> Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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(at)sraoss(dot)co(dot)jp>
>
>

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>