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
diff --git a/src/include/fe_utils/cancel.h b/src/include/fe_utils/cancel.h
index 959a38acf3..24b7001db7 100644
--- a/src/include/fe_utils/cancel.h
+++ b/src/include/fe_utils/cancel.h
@@ -14,9 +14,11 @@
#ifndef CANCEL_H
#define CANCEL_H
+#include <signal.h>
+
#include "libpq-fe.h"
-extern bool CancelRequested;
+extern volatile sig_atomic_t CancelRequested;
extern void SetCancelConn(PGconn *conn);
extern void ResetCancelConn(void);
diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
index f138d963d3..7a280eaebd 100644
--- a/src/include/fe_utils/print.h
+++ b/src/include/fe_utils/print.h
@@ -13,6 +13,8 @@
#ifndef PRINT_H
#define PRINT_H
+#include <signal.h>
+
#include "libpq-fe.h"
@@ -175,7 +177,7 @@ typedef struct printQueryOpt
} printQueryOpt;
-extern volatile bool cancel_pressed;
+extern volatile sig_atomic_t cancel_pressed;
extern const printTextFormat pg_asciiformat;
extern const printTextFormat pg_asciiformat_old;
diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c
index 04e0d1e3b2..9c3922b5c4 100644
--- a/src/fe_utils/cancel.c
+++ b/src/fe_utils/cancel.c
@@ -16,7 +16,6 @@
#include "postgres_fe.h"
-#include <signal.h>
#include <unistd.h>
#include "fe_utils/cancel.h"
@@ -37,8 +36,20 @@
(void) rc_; \
} while (0)
+/*
+ * Contains all the information needed to cancel a query issued from
+ * a database connection to the backend.
+ */
static PGcancel *volatile cancelConn = NULL;
-bool CancelRequested = false;
+
+/*
+ * CancelRequested tracks if a cancellation request has completed after
+ * a signal interruption. Note that if cancelConn is not set, in short
+ * if SetCancelConn() was never called or if ResetCancelConn() freed
+ * the cancellation object, then CancelRequested is switched to true after
+ * all cancellation attempts.
+ */
+volatile sig_atomic_t CancelRequested = false;
#ifdef WIN32
static CRITICAL_SECTION cancelConnLock;
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index b9cd6a1752..bcc77f471c 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -19,7 +19,6 @@
#include <limits.h>
#include <math.h>
-#include <signal.h>
#include <unistd.h>
#ifndef WIN32
@@ -41,7 +40,7 @@
* Note: print.c's general strategy for when to check cancel_pressed is to do
* so at completion of each row of output.
*/
-volatile bool cancel_pressed = false;
+volatile sig_atomic_t cancel_pressed = false;
static bool always_ignore_sigpipe = false;
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9c0d53689e..39dc805d7a 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -233,7 +233,7 @@ NoticeProcessor(void *arg, const char *message)
*
* SIGINT is supposed to abort all long-running psql operations, not only
* database queries. In most places, this is accomplished by checking
- * CancelRequested during long-running loops. However, that won't work when
+ * cancel_pressed during long-running loops. However, that won't work when
* blocked on user input (in readline() or fgets()). In those places, we
* set sigint_interrupt_enabled true while blocked, instructing the signal
* catcher to longjmp through sigint_interrupt_jmp. We assume readline and
@@ -246,32 +246,27 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-#ifndef WIN32
-
static void
psql_cancel_callback(void)
{
+#ifndef WIN32
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
sigint_interrupt_enabled = false;
siglongjmp(sigint_interrupt_jmp, 1);
}
+#endif
- /* else, set cancel flag to stop any long-running loops */
- CancelRequested = true;
+ /*
+ * Else, set cancel flag to stop any long-running loops. Note that
+ * CancelRequested is not used here as this flag in the frontend
+ * cancellation facility in src/fe_utils/cancel.c tracks if a
+ * cancellation request has succeeded.
+ */
+ cancel_pressed = true;
}
-#else
-
-static void
-psql_cancel_callback(void)
-{
- /* nothing to do here */
-}
-
-#endif /* !WIN32 */
-
void
psql_setup_cancel_handler(void)
{
@@ -638,7 +633,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt)
* consumed. The user's intention, though, is to cancel the entire watch
* process, so detect a sent cancellation request and exit in this case.
*/
- if (CancelRequested)
+ if (cancel_pressed)
{
PQclear(res);
return 0;
@@ -838,8 +833,8 @@ ExecQueryTuples(const PGresult *result)
{
const char *query = PQgetvalue(result, r, c);
- /* Abandon execution if CancelRequested */
- if (CancelRequested)
+ /* Abandon execution if cancel_pressed */
+ if (cancel_pressed)
goto loop_exit;
/*
@@ -1207,7 +1202,7 @@ SendQuery(const char *query)
if (fgets(buf, sizeof(buf), stdin) != NULL)
if (buf[0] == 'x')
goto sendquery_cleanup;
- if (CancelRequested)
+ if (cancel_pressed)
goto sendquery_cleanup;
}
else if (pset.echo == PSQL_ECHO_QUERIES)
@@ -1751,7 +1746,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
* writing things to the stream, we presume $PAGER has disappeared and
* stop bothering to pull down more data.
*/
- if (ntuples < fetch_count || CancelRequested || flush_error ||
+ if (ntuples < fetch_count || cancel_pressed || flush_error ||
ferror(fout))
break;
}
signature.asc
Description: PGP signature
