Hello Tom,
My 0.02 €:
Given the rather small number of existing uses of CancelRequested,
I wonder if it wouldn't be a better idea to rename it to cancel_pressed?
I prefer the former because it is more functional (a cancellation has been
requested, however the mean to do so) while "pressed" rather suggest a
particular operation.
Also, perhaps I am missing something, but I do not see anyplace in the
current code base that ever *clears* CancelRequested.
This was already like that in the initial version before the refactoring.
./src/bin/scripts/common.h:extern bool CancelRequested;
./src/bin/scripts/common.c:bool CancelRequested = false;
./src/bin/scripts/common.c: CancelRequested = true;
./src/bin/scripts/common.c: CancelRequested = true;
./src/bin/scripts/common.c: CancelRequested = true;
./src/bin/scripts/common.c: CancelRequested = true;
./src/bin/scripts/vacuumdb.c: if (CancelRequested)
./src/bin/scripts/vacuumdb.c: if (CancelRequested)
./src/bin/scripts/vacuumdb.c: if (i < 0 || CancelRequested)
However "cancel_request" resets are in "psql/mainloop.c", and are changed
to "CancelRequest = false" resets by Michaël patch, so all seems fine.
How much has this code been tested? 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?
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.
--
Fabien.
