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??
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.
Should PQfreeCancel be called on deconnections, in finishCon? I think that
there may be a memory leak with the current implementation??
Maybe it should check that cancel is not NULL before calling PQcancel?
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.
--
Fabien.
