Fix query cancellation handling in psql

The refactoring done in a4fd3aa for query cancellation has messed up
with the logic in psql by mixing CancelRequested and cancel_pressed,
breaking for example \watch.  The former would be switched to true if a
cancellation request has been attempted and that it actually succeeded,
and the latter tracks if a cancellation attempt has been done.

This commit brings back the code of psql to a state consistent to what
it was before a4fd3aa, without giving up on the refactoring pieces
introduced.  It should be actually possible to merge more both flags as
their concepts are close enough, however note that psql's --single-step
mode relies on cancel_pressed to be always set, so this requires more
careful analysis left for later.

While on it, fix the declarations of CancelRequested (in cancel.c) and
cancel_pressed (in psql) to be volatile sig_atomic_t.  Previously,
both were declared as booleans, which should be fine on modern
platforms, but the C standard recommends the use of sig_atomic_t for
variables used in signal handlers.  Note that since its introduction in
a1792320, CancelRequested declaration was not volatile.

Reported-by: Jeff Janes
Author: Michael Paquier
Discussion: https://postgr.es/m/CAMkU=1zpoUDGKqWKuMWkj7t-bOCaJDx0r=5te_-d0B2HVLABXg@mail.gmail.com

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9c0d53689ec4d519b25837d7448ee26f9c45ea8a..9aef756f25cf61035155062c2841135b0b3d1118 100644 (file)
--- 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,22 @@ 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
-
-static void
-psql_cancel_callback(void)
-{
-       /* nothing to do here */
+       cancel_pressed = true;
 }
 
-#endif                                                 /* !WIN32 */
-
 void
 psql_setup_cancel_handler(void)
 {
@@ -638,7 +628,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 +828,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 +1197,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 +1741,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;
        }

diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c
index 04e0d1e3b2d52c0690c13659d8256ee7daa0f882..9c3922b5c404d1d6bfb9fce90a55e22495d3d969 100644 (file)
--- 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 b9cd6a17521be3fd73ba0844573912b3af12e4e3..bcc77f471cf1327d4fc26a2d9dd82b6e64329bef 100644 (file)
--- 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/include/fe_utils/cancel.h b/src/include/fe_utils/cancel.h
index 959a38acf33926e00e784d8da9030321d5d63fe1..24b7001db79df71f8f06dc07bce1ee69b8dea688 100644 (file)
--- 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 f138d963d3f8d392c47eaa21f248216dad3b1959..7a280eaebd64795cb5b66fa79e9fe7e5f931cc13 100644 (file)
--- 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;