Re: psql's \watch is broken

On Sun, Dec 15, 2019 at 10:35:54PM +0100, Fabien COELHO wrote:
>> Also, perhaps I am missing something, but I do not see anyplace in the
>> current code base that ever *clears* CancelRequested.

For bin/scripts/, that's not really a problem, because all code paths
triggering a cancellation, aka in vacuumdb and reindexdb, exit
immediately.

>> How much has this code been tested?

Sorry about that, not enough visibly :(

>> Is it really sane to remove the setting of that flag from
>> psql_cancel_callback, as this patch does?
>
> ISTM that the callback is called from a function which sets CancelRequest?

Hmm, that's not right.  Note that there is a subtle difference between
psql and bin/scripts/.  In the case of the scripts, originally
CancelRequested tracks if a cancellation request has been sent to the
backend or not.  Hence, if the client has not called SetCancelConn()
to set up cancelConn, then CancelRequested is switched to true all the
time.  Now, if cancelConn is set, but a cancellation request has not
correctly completed, then CancelRequested never set to true.

In the case of psql, the original code sets cancel_pressed all the
time, even if a cancellation request has been done and that it failed,
and did not care if cancelConn was set or not.  So, the intention of
psql is to always track when a cancellation attempt is done, even if
it has failed to issue it, while for all our other frontends we want
to make sure that a cancellation attempt is done, and that the
cancellation has succeeded before looping out and exit.

>>  Is it sane that CancelRequested isn't declared volatile?
>
> I agree that it would seem appropriate, but the initial version I refactored
> was not declaring CancelRequested as volatile, so I did not change that.
> However, "cancel_pressed" is volatile, so merging the two
> would indeed suggest to declare it as volatile.

Actually, it seems to me that both suggestions are not completely
right either about that stuff since the flag has been introduced in
bin/scripts/ in a1792320, no?  The way to handle such variables safely
in a signal handler it to mark them as volatile and sig_atomic_t.  The
same can be said about the older cancel_pressed as of 718bb2c in psql.
So fixed all that while on it.

As the concepts behind cancel_pressed and CancelRequested are
different, we need to keep cancel_pressed and make psql use it.  And
the callback used for WIN32 also needs to set the flag. I also think
that we should do a better effort in documenting CancelRequested
properly in cancel.c.  All that should be fixed as of the attached,
tested on Linux and from a Windows console.  From a point of view of
consistency, this actually brings back the code of psql to the same
state as it was before a4fd3aa, except that we still have the
refactored pieces.

Thoughts?
--
Michael