Re: fe-utils - share query cancellation code
On Tue, Dec 03, 2019 at 01:11:27PM +0100, Fabien COELHO wrote: > Looks fine to me: patch applies, compiles, runs. Thanks for double-checking. Done. -- Michael signature.asc Description: PGP signature
Re: fe-utils - share query cancellation code
Bonjour Michaël, Committed the patch after splitting things into two commits and after testing things from Linux and from a Windows console: the actual refactoring and the pgbench changes. I have found that we have a useless declaration of CancelRequested in common.h, which is already part of cancel.h. Ok. On top of that I think that we need to rework a bit the header inclusions of bin/scripts/, as per the attached. Looks fine to me: patch applies, compiles, runs. -- Fabien.
Re: fe-utils - share query cancellation code
On Mon, Dec 02, 2019 at 11:54:02AM +0900, Michael Paquier wrote: > Committed the patch after splitting things into two commits and after > testing things from Linux and from a Windows console: the actual > refactoring and the pgbench changes. I have found that we have a useless declaration of CancelRequested in common.h, which is already part of cancel.h. On top of that I think that we need to rework a bit the header inclusions of bin/scripts/, as per the attached. A small set of issues, still these are issues. Sorry for having missed these. -- Michael diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c index 3aee5f2834..936dd62052 100644 --- a/src/bin/scripts/clusterdb.c +++ b/src/bin/scripts/clusterdb.c @@ -12,6 +12,7 @@ #include "postgres_fe.h" #include "common.h" #include "common/logging.h" +#include "fe_utils/cancel.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c index d2a7547441..680bbb133a 100644 --- a/src/bin/scripts/common.c +++ b/src/bin/scripts/common.c @@ -19,6 +19,7 @@ #include "common.h" #include "common/logging.h" +#include "fe_utils/cancel.h" #include "fe_utils/connect.h" #include "fe_utils/string_utils.h" diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h index db2f85b472..33b952ba63 100644 --- a/src/bin/scripts/common.h +++ b/src/bin/scripts/common.h @@ -10,7 +10,6 @@ #define COMMON_H #include "common/username.h" -#include "fe_utils/cancel.h" #include "getopt_long.h" /* pgrminclude ignore */ #include "libpq-fe.h" #include "pqexpbuffer.h" /* pgrminclude ignore */ @@ -22,8 +21,6 @@ enum trivalue TRI_YES }; -extern bool CancelRequested; - typedef void (*help_handler) (const char *progname); extern void handle_help_version_opts(int argc, char *argv[], diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c index bedd95cf9d..65abe5984e 100644 --- a/src/bin/scripts/reindexdb.c +++ b/src/bin/scripts/reindexdb.c @@ -14,6 +14,7 @@ #include "catalog/pg_class_d.h" #include "common.h" #include "common/logging.h" +#include "fe_utils/cancel.h" #include "fe_utils/connect.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" diff --git a/src/bin/scripts/scripts_parallel.c b/src/bin/scripts/scripts_parallel.c index 97435160e9..a732f07dd8 100644 --- a/src/bin/scripts/scripts_parallel.c +++ b/src/bin/scripts/scripts_parallel.c @@ -24,6 +24,7 @@ #include "common.h" #include "common/logging.h" +#include "fe_utils/cancel.h" #include "scripts_parallel.h" static void init_slot(ParallelSlot *slot, PGconn *conn); diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c index 83a94dc632..e1623e76e3 100644 --- a/src/bin/scripts/vacuumdb.c +++ b/src/bin/scripts/vacuumdb.c @@ -16,6 +16,7 @@ #include "common.h" #include "common/logging.h" +#include "fe_utils/cancel.h" #include "fe_utils/connect.h" #include "fe_utils/simple_list.h" #include "fe_utils/string_utils.h" signature.asc Description: PGP signature
Re: fe-utils - share query cancellation code
On Fri, Nov 29, 2019 at 08:44:25AM +0100, Fabien COELHO wrote: > I do not have a Windows host, so I can only do blind tests. I just moved the > declaration out of !WIN32 scope in attached v7, which might solve the > resolution error. All other issues pointed out above seem fixed in the v6 > you sent. Committed the patch after splitting things into two commits and after testing things from Linux and from a Windows console: the actual refactoring and the pgbench changes. While polishing the code, I have found the upthread argument of Ibrar quite appealing because there are use cases where a callback can be interesting on Windows, like simply being able to log the cancel event to a different source. So I have removed the callback restriction and the assertion, making the callback of psql a no-op on Windows. A second thing is that two large comments originally in psql had better be moved to cancel.c because the logic with libpq cancel routines applies only there. -- Michael signature.asc Description: PGP signature
Re: fe-utils - share query cancellation code
Bonjour Michaël,
The query cancellation added to pgbench is different than the actual
refactoring, and it is a result of the refactoring, so I would rather
split that into two different commits for clarity. The split is easy
enough, so that's fine not to send two different patches.
Yep, different file set.
Compilation of the patch fails for me on Windows for psql:
unresolved external symbol sigint_interrupt_jmp
Please note that Mr Robot complains as well at build time:
http://commitfest.cputube.org/fabien-coelho.html
Visibly the problem here is that sigint_interrupt_jmp is declared in
common.h, but you have moved it to a non-WIN32 section of the code in
psql/common.c. And actually, note that copy.c and mainloop.c make use
of it...
Indeed.
I would not worry much about SIGTERM as you mentioned in the comments,
query cancellations are associated to SIGINT now. There could be an
argument possible later to allow passing down a custom signal though.
Ok.
Attached is an updated patch with a couple of edits I have done,
including the removal of some noise diffs and the previous edits.
Thanks!
I am switching the patch as waiting on author, bumping it to next CF at
the same time. Could you please fix the Windows issue?
I do not have a Windows host, so I can only do blind tests. I just moved
the declaration out of !WIN32 scope in attached v7, which might solve the
resolution error. All other issues pointed out above seem fixed in the v6
you sent.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4a7ac1f821..5129aea516 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,6 +59,7 @@
#include "common/int.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -3894,6 +3895,9 @@ initGenerateDataClientSide(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4109,6 +4113,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4176,6 +4183,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer(&stats);
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 0a2597046d..48b6279403 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "describe.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
#include "help.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 53a1ea2bdb..442ff2fe5d 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -23,6 +23,7 @@
#include "common/logging.h"
#include "copy.h"
#include "crosstabview.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/mbprint.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
@@ -241,7 +242,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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
@@ -252,34 +253,11 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
#ifndef WIN32
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +266,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof
Re: fe-utils - share query cancellation code
On Wed, Nov 06, 2019 at 10:41:39AM +0100, Fabien COELHO wrote:
> Indeed, I put it on the wrong side of a "#ifndef WIN32".
>
> Basically it is a false constant under WIN32, which it seems does not have
> sigint handler, but the code checks whether the non existent handler is
> enabled anyway.
>
> Patch v5 attached fixes that, hopefully.
I have looked at this one, and found a couple of issues, most of them
small-ish.
s/cancelation/cancellation/ in fe_utils/cancel.h.
Then, the format of the new file headers was not really consistent
with the rest, and the order of the headers included in most of the
files was incorrect. That would break the recent flow of commits done
by Amit K.
The query cancellation added to pgbench is different than the actual
refactoring, and it is a result of the refactoring, so I would rather
split that into two different commits for clarity. The split is easy
enough, so that's fine not to send two different patches.
Compilation of the patch fails for me on Windows for psql:
unresolved external symbol sigint_interrupt_jmp
Please note that Mr Robot complains as well at build time:
http://commitfest.cputube.org/fabien-coelho.html
Visibly the problem here is that sigint_interrupt_jmp is declared in
common.h, but you have moved it to a non-WIN32 section of the code in
psql/common.c. And actually, note that copy.c and mainloop.c make use
of it...
I would not worry much about SIGTERM as you mentioned in the comments,
query cancellations are associated to SIGINT now. There could be an
argument possible later to allow passing down a custom signal though.
Attached is an updated patch with a couple of edits I have done,
including the removal of some noise diffs and the previous edits. I
am switching the patch as waiting on author, bumping it to next CF at
the same time. Could you please fix the Windows issue?
--
Michael
diff --git a/src/include/fe_utils/cancel.h b/src/include/fe_utils/cancel.h
new file mode 100644
index 00..cc199b7c8a
--- /dev/null
+++ b/src/include/fe_utils/cancel.h
@@ -0,0 +1,34 @@
+/*-
+ *
+ * Query cancellation support for frontend code
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/fe_utils/cancel.h
+ *
+ *-
+ */
+
+#ifndef CANCEL_H
+#define CANCEL_H
+
+#include "libpq-fe.h"
+
+extern bool CancelRequested;
+
+extern void SetCancelConn(PGconn *conn);
+extern void ResetCancelConn(void);
+
+#ifndef WIN32
+extern void setup_cancel_handler(void (*callback)(void));
+#else
+/*
+ * Ensure that the signature is the same under windows, at the price of
+ * ignoring the function parameter.
+ */
+extern void setup_cancel_handler(void *ignored);
+#endif /* WIN32 */
+
+#endif /* CANCEL_H */
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index beea506536..e029333194 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -20,6 +20,7 @@ include $(top_builddir)/src/Makefile.global
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
OBJS = \
+ cancel.o \
conditional.o \
mbprint.o \
print.o \
diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c
new file mode 100644
index 00..71d0911236
--- /dev/null
+++ b/src/fe_utils/cancel.c
@@ -0,0 +1,208 @@
+/*
+ *
+ * Query cancellation support for frontend code
+ *
+ * Assorted utility functions to control query cancellation with signal
+ * handler for SIGINT.
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/fe-utils/cancel.c
+ *
+ *
+ */
+
+#include "postgres_fe.h"
+
+#include
+#include
+
+#include "fe_utils/cancel.h"
+#include "fe_utils/connect.h"
+#include "fe_utils/string_utils.h"
+
+
+/*
+ * Write a simple string to stderr --- must be safe in a signal handler.
+ * We ignore the write() result since there's not much we could do about it.
+ * Certain compilers make that harder than it ought to be.
+ */
+#define write_stderr(str) \
+ do { \
+ const char *str_ = (str); \
+ int rc_; \
+ rc_ = write(fileno(stderr), str_, strlen(str_)); \
+ (void) rc_; \
+ } while (0)
+
+static PGcancel *volatile cancelConn = NULL;
+bool CancelRequested = false;
+
+#ifdef WIN32
+static CRITICAL_SECTION cancelConnLock;
+#endif
+
+/*
+ * SetCancelConn
+ *
+ * Set cancelConn to point to the current database connection.
+ */
+void
+SetCancelConn(PGconn *conn)
+{
+ PGcancel *oldCancelConn;
+
+#ifdef WIN32
+ EnterCriticalSection(&cancelConnLock);
+#endif
+
+ /* Free the old one if we have one */
+ oldCancelConn = cancelConn;
+ /* be sure handle_sigint doesn't use poi
Re: fe-utils - share query cancellation code
It looks like don't define sigint_interrupt_jmp and
sigint_interrupt_enabled on Windows, yet they are still declared and
referenced?
Indeed, I put it on the wrong side of a "#ifndef WIN32".
Basically it is a false constant under WIN32, which it seems does not have
sigint handler, but the code checks whether the non existent handler is
enabled anyway.
Patch v5 attached fixes that, hopefully.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 14dbc4510c..b5e3533a07 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,6 +59,7 @@
#include "common/int.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -3894,6 +3895,9 @@ initGenerateDataClientSide(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4109,6 +4113,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4176,6 +4183,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer(&stats);
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae81ff..f1d9e0298a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 90f6380170..c685e072ef 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -24,6 +24,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "fe_utils/mbprint.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -223,7 +224,6 @@ NoticeProcessor(void *arg, const char *message)
}
-
/*
* Code to support query cancellation
*
@@ -241,45 +241,23 @@ 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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
* fgets are coded to handle possible interruption. (XXX currently this does
* not work on win32, so control-C is less useful there)
*/
+
volatile bool sigint_interrupt_enabled = false;
+#ifndef WIN32
+
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
-#ifndef WIN32
-
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +266,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
- write_stderr("Cancel request sent\n");
- else
- {
- write_stderr("Could not send cancel request: ");
- write_stderr(errbuf);
- }
- }
-
- errno = save_errno; /* just in case the write changed it */
+ CancelRequested = true;
}
-void
-setup_cancel_handler(void)
-{
- pqsignal(SIGINT, handle_sigint);
-}
-#else /* WIN32 */
-
-static BOOL WINAPI
-consoleHandler(DWORD dwCtrlType)
-{
- char errbuf[256];
-
- if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT)
- {
- /*
- * Can't longjmp here, because we are in wrong thread :-(
- */
-
- /* set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- EnterCriticalSection(&cancelConnLock);
- if (cancelConn != NULL)
- {
- if (PQcancel(cance
Re: fe-utils - share query cancellation code
On Sat, Nov 2, 2019 at 10:38 AM Fabien COELHO wrote: > Attached patch v4 does it. Hi Fabien, It looks like don't define sigint_interrupt_jmp and sigint_interrupt_enabled on Windows, yet they are still declared and referenced? command.obj : error LNK2001: unresolved external symbol sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj] copy.obj : error LNK2001: unresolved external symbol sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj] input.obj : error LNK2001: unresolved external symbol sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj] mainloop.obj : error LNK2001: unresolved external symbol sigint_interrupt_enabled [C:\projects\postgresql\psql.vcxproj] command.obj : error LNK2001: unresolved external symbol sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj] copy.obj : error LNK2001: unresolved external symbol sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj] mainloop.obj : error LNK2001: unresolved external symbol sigint_interrupt_jmp [C:\projects\postgresql\psql.vcxproj] .\Release\psql\psql.exe : fatal error LNK1120: 2 unresolved externals [C:\projects\postgresql\psql.vcxproj] 0 Warning(s) https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64074
Re: fe-utils - share query cancellation code
Then you need to add #include libpq-fe.h in cancel.h. Our policy is
that headers compile standalone (c.h / postgres_fe.h / postgres.h
excluded).
Ok. I do not make a habit of adding headers in postgres, so I did not
notice there was an alphabetical logic to that.
Attached patch v4 does it.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..904d6f0e00 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,6 +59,7 @@
#include "common/int.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -3887,6 +3888,9 @@ initGenerateData(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4057,6 +4061,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4120,6 +4127,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer(&stats);
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae81ff..f1d9e0298a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 90f6380170..0a66b71372 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -24,6 +24,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "fe_utils/mbprint.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -223,6 +224,7 @@ NoticeProcessor(void *arg, const char *message)
}
+#ifndef WIN32
/*
* Code to support query cancellation
@@ -241,7 +243,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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
@@ -252,34 +254,9 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
-#ifndef WIN32
-
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +265,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
- write_stderr("Cancel request sent\n");
- else
- {
- write_stderr("Could not send cancel request: ");
- write_stderr(errbuf);
- }
- }
-
- errno = save_errno; /* just in case the write changed it */
+ CancelRequested = true;
}
-void
-setup_cancel_handler(void)
-{
- pqsignal(SIGINT, handle_sigint);
-}
-#else /* WIN32 */
-
-static BOOL WINAPI
-consoleHandler(DWORD dwCtrlType)
-{
- char errbuf[256];
-
- if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT)
- {
- /*
- * Can't longjmp here, because we are in wrong thread :-(
- */
-
- /* set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- EnterCriticalSection(&cancelConnLock);
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
-write_stderr("Cancel request sent\n");
- else
- {
-write_stderr("Could not send cancel request: ");
-write_stderr(errbuf);
- }
- }
- LeaveCriticalSection(&cancelConnLock);
-
- return TRUE;
- }
-
Re: fe-utils - share query cancellation code
On 2019-Nov-01, Fabien COELHO wrote: > Because "cancel.h" requires PGconn declaration, thus must appear after > "libpq-fe.h", Then you need to add #include libpq-fe.h in cancel.h. Our policy is that headers compile standalone (c.h / postgres_fe.h / postgres.h excluded). -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: fe-utils - share query cancellation code
I understand that you are unhappy about something, but where the issue is
fails me, the "wtf" 3 characters are not enough to point me in the right
direction. Feel free to elaborate a little bit more:-)
I don't see why you move the "conditional.h" line out of its correct
alphabetical position (where it is now), and then add "cancel.h" next to
it also out of its correct alphabetical position.
Because "cancel.h" requires PGconn declaration, thus must appear after
"libpq-fe.h", and once I put it after that letting "conditional.h" above &
alone looked a little bit silly. I put cancel after conditional because it
was the new addition, which is somehow logical, although not alphabetical.
Now I can put cancel before conditional, sure.
Patch v3 attached does that.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..7aab4bdf47 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,9 +59,10 @@
#include "common/int.h"
#include "common/logging.h"
-#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
+#include "fe_utils/cancel.h"
+#include "fe_utils/conditional.h"
#include "pgbench.h"
#include "portability/instr_time.h"
@@ -3887,6 +3888,9 @@ initGenerateData(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4057,6 +4061,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4120,6 +4127,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer(&stats);
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae81ff..f1d9e0298a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 90f6380170..0a66b71372 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -24,6 +24,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "fe_utils/mbprint.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -223,6 +224,7 @@ NoticeProcessor(void *arg, const char *message)
}
+#ifndef WIN32
/*
* Code to support query cancellation
@@ -241,7 +243,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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
@@ -252,34 +254,9 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
-#ifndef WIN32
-
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +265,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
- write_stderr("Cancel request sent\n");
- else
- {
- write_stderr("Could not send cancel request: ");
- write_stderr(errbuf);
- }
- }
-
- errno = save_errno; /* just in case the write changed it */
+ CancelRequested = true;
}
-void
-setup_cancel_handler(void)
-{
- pqsignal(SIGINT, handle_sigint);
-}
-#else /* WIN32 */
-
-static BOOL WINAPI
-consoleHandler(DWORD dwCtrlType)
-{
- char errbuf[256];
-
- if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT)
Re: fe-utils - share query cancellation code
On 2019-Nov-01, Fabien COELHO wrote: > > > #include "common/int.h" > > > #include "common/logging.h" > > > -#include "fe_utils/conditional.h" > > > #include "getopt_long.h" > > > #include "libpq-fe.h" > > > +#include "fe_utils/conditional.h" > > > +#include "fe_utils/cancel.h" > > > #include "pgbench.h" > > > #include "portability/instr_time.h" > > > > wtf? > > I understand that you are unhappy about something, but where the issue is > fails me, the "wtf" 3 characters are not enough to point me in the right > direction. Feel free to elaborate a little bit more:-) I don't see why you move the "conditional.h" line out of its correct alphabetical position (where it is now), and then add "cancel.h" next to it also out of its correct alphabetical position. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: fe-utils - share query cancellation code
Hello Alvaro, #include "common/int.h" #include "common/logging.h" -#include "fe_utils/conditional.h" #include "getopt_long.h" #include "libpq-fe.h" +#include "fe_utils/conditional.h" +#include "fe_utils/cancel.h" #include "pgbench.h" #include "portability/instr_time.h" wtf? I understand that you are unhappy about something, but where the issue is fails me, the "wtf" 3 characters are not enough to point me in the right direction. Feel free to elaborate a little bit more:-) -- Fabien.
Re: fe-utils - share query cancellation code
On 2019-Nov-01, Fabien COELHO wrote: > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c > index 03bcd22996..389b4d7bcd 100644 > --- a/src/bin/pgbench/pgbench.c > +++ b/src/bin/pgbench/pgbench.c > @@ -59,9 +59,10 @@ > > #include "common/int.h" > #include "common/logging.h" > -#include "fe_utils/conditional.h" > #include "getopt_long.h" > #include "libpq-fe.h" > +#include "fe_utils/conditional.h" > +#include "fe_utils/cancel.h" > #include "pgbench.h" > #include "portability/instr_time.h" wtf? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: fe-utils - share query cancellation code
Hello,
I give a quick look and I think we can
void
psql_setup_cancel_handler(void)
{
setup_cancel_handler(psql_sigint_callback);
}
Because it does not matter for setup_cancel_handler what we passed
because it is ignoring that in case of windows.
The "psql_sigint_callback" function is not defined under WIN32.
I've fixed a missing NULL argument in the section you pointed out, though.
I've used the shared infrastructure in pgbench.
I've noticed yet another instance of the cancelation stuff in
"src/bin/pg_dump/parallel.c", but it seems somehow different from the two
others, so I have not tried to used the shared version.
--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 03bcd22996..389b4d7bcd 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -59,9 +59,10 @@
#include "common/int.h"
#include "common/logging.h"
-#include "fe_utils/conditional.h"
#include "getopt_long.h"
#include "libpq-fe.h"
+#include "fe_utils/conditional.h"
+#include "fe_utils/cancel.h"
#include "pgbench.h"
#include "portability/instr_time.h"
@@ -3887,6 +3888,9 @@ initGenerateData(PGconn *con)
exit(1);
}
+ if (CancelRequested)
+ break;
+
/*
* If we want to stick with the original logging, print a message each
* 100k inserted rows.
@@ -4057,6 +4061,9 @@ runInitSteps(const char *initialize_steps)
if ((con = doConnect()) == NULL)
exit(1);
+ setup_cancel_handler(NULL);
+ SetCancelConn(con);
+
for (step = initialize_steps; *step != '\0'; step++)
{
instr_time start;
@@ -4120,6 +4127,7 @@ runInitSteps(const char *initialize_steps)
}
fprintf(stderr, "done in %.2f s (%s).\n", run_time, stats.data);
+ ResetCancelConn();
PQfinish(con);
termPQExpBuffer(&stats);
}
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae81ff..f1d9e0298a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 90f6380170..0a66b71372 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -24,6 +24,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "fe_utils/mbprint.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -223,6 +224,7 @@ NoticeProcessor(void *arg, const char *message)
}
+#ifndef WIN32
/*
* Code to support query cancellation
@@ -241,7 +243,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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
@@ -252,34 +254,9 @@ volatile bool sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
-#ifndef WIN32
-
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +265,20 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
- write_stderr("Cancel request sent\n");
- else
- {
- write_stderr("Could not send cancel request: ");
- write_stderr(errbuf);
- }
- }
-
- errno = save_errno; /* just in case the write changed it */
+ CancelRequested = true;
}
-void
-setup_cancel_handler(void)
-{
- pqsignal(SIGINT, handle_sigint);
-}
-#else /* WIN32 */
-
-static BOOL WINAPI
-consoleHandler(DWORD dwCtrlType)
-{
- char errbuf[256];
-
- if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT)
- {
- /*
- * Can't longjmp here, because we are in wrong thread :-(
- */
-
- /* set cancel flag to stop any long-running loops
Re: fe-utils - share query cancellation code
On Thu, Oct 31, 2019 at 11:43 PM Fabien COELHO wrote:
>
> Hello Devs,
>
> This patch moves duplicated query cancellation code code from psql &
> scripts to fe-utils, so that it is shared and may be used by other
> commands.
>
> This is because Masao-san suggested to add a query cancellation feature to
> pgbench for long queries (server-side data generation being discussed, but
> possibly pk and fk could use that as well).
>
> --
> Fabien.
I give a quick look and I think we can
void
psql_setup_cancel_handler(void)
{
#ifndef WIN32
setup_cancel_handler(psql_sigint_callback);
#else
setup_cancel_handler();
#endif /* WIN32 */
}
to
void
psql_setup_cancel_handler(void)
{
setup_cancel_handler(psql_sigint_callback);
}
Because it does not matter for setup_cancel_handler what we passed
because it is ignoring that in case of windows.
Hmm, need to remove the assert in the function
"setup_cancel_handler"
-- Ibrar Ahmed
fe-utils - share query cancellation code
Hello Devs,
This patch moves duplicated query cancellation code code from psql &
scripts to fe-utils, so that it is shared and may be used by other
commands.
This is because Masao-san suggested to add a query cancellation feature to
pgbench for long queries (server-side data generation being discussed, but
possibly pk and fk could use that as well).
--
Fabien.diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b981ae81ff..f1d9e0298a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -29,6 +29,7 @@
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 90f6380170..a00990d214 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -24,6 +24,7 @@
#include "copy.h"
#include "crosstabview.h"
#include "fe_utils/mbprint.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/string_utils.h"
#include "portability/instr_time.h"
#include "settings.h"
@@ -222,7 +223,7 @@ NoticeProcessor(void *arg, const char *message)
pg_log_info("%s", message);
}
-
+#ifndef WIN32
/*
* Code to support query cancellation
@@ -241,7 +242,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
- * cancel_pressed during long-running loops. However, that won't work when
+ * CancelRequested 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
@@ -249,37 +250,11 @@ NoticeProcessor(void *arg, const char *message)
* not work on win32, so control-C is less useful there)
*/
volatile bool sigint_interrupt_enabled = false;
-
sigjmp_buf sigint_interrupt_jmp;
-static PGcancel *volatile cancelConn = NULL;
-
-#ifdef WIN32
-static CRITICAL_SECTION cancelConnLock;
-#endif
-
-/*
- * Write a simple string to stderr --- must be safe in a signal handler.
- * We ignore the write() result since there's not much we could do about it.
- * Certain compilers make that harder than it ought to be.
- */
-#define write_stderr(str) \
- do { \
- const char *str_ = (str); \
- int rc_; \
- rc_ = write(fileno(stderr), str_, strlen(str_)); \
- (void) rc_; \
- } while (0)
-
-
-#ifndef WIN32
-
static void
-handle_sigint(SIGNAL_ARGS)
+psql_sigint_callback(void)
{
- int save_errno = errno;
- char errbuf[256];
-
/* if we are waiting for input, longjmp out of it */
if (sigint_interrupt_enabled)
{
@@ -288,74 +263,19 @@ handle_sigint(SIGNAL_ARGS)
}
/* else, set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
- write_stderr("Cancel request sent\n");
- else
- {
- write_stderr("Could not send cancel request: ");
- write_stderr(errbuf);
- }
- }
-
- errno = save_errno; /* just in case the write changed it */
-}
-
-void
-setup_cancel_handler(void)
-{
- pqsignal(SIGINT, handle_sigint);
-}
-#else /* WIN32 */
-
-static BOOL WINAPI
-consoleHandler(DWORD dwCtrlType)
-{
- char errbuf[256];
-
- if (dwCtrlType == CTRL_C_EVENT ||
- dwCtrlType == CTRL_BREAK_EVENT)
- {
- /*
- * Can't longjmp here, because we are in wrong thread :-(
- */
-
- /* set cancel flag to stop any long-running loops */
- cancel_pressed = true;
-
- /* and send QueryCancel if we are processing a database query */
- EnterCriticalSection(&cancelConnLock);
- if (cancelConn != NULL)
- {
- if (PQcancel(cancelConn, errbuf, sizeof(errbuf)))
-write_stderr("Cancel request sent\n");
- else
- {
-write_stderr("Could not send cancel request: ");
-write_stderr(errbuf);
- }
- }
- LeaveCriticalSection(&cancelConnLock);
-
- return TRUE;
- }
- else
- /* Return FALSE for any signals not being handled */
- return FALSE;
+ CancelRequested = true;
}
+#endif
void
-setup_cancel_handler(void)
+psql_setup_cancel_handler(void)
{
- InitializeCriticalSection(&cancelConnLock);
-
- SetConsoleCtrlHandler(consoleHandler, TRUE);
+#ifndef WIN32
+ setup_cancel_handler(psql_sigint_callback);
+#else
+ setup_cancel_handler();
+#endif /* WIN32 */
}
-#endif /* WIN32 */
/* ConnectionUp
@@ -369,7 +289,6 @@ ConnectionUp(void)
}
-
/* CheckConnection
*
* Verify that we still have a good connection to the backend, and if not,
@@ -428,62 +347,6 @@ CheckConnection(void)
-/*
- * SetCancelConn
- *
- * Set cancelConn to point to the current database connection.
- */
-void
-SetCancelConn(void)
-{
- PGcancel *oldCancelConn;
-
-#ifdef WIN32
- EnterCriticalSection(&cancel
