psql's \watch is broken
If I do something like this:
explain (analyze) select * from pgbench_accounts \watch 1
It behaves as expected. But once I break out of the loop with ctrl-C, then
if I execute the same thing again it executes the command once, but shows
no output and doesn't loop. It seems like some flag is getting set with
ctrl-C, but then never gets reset.
It was broken in this commit:
commit a4fd3aa719e8f97299dfcf1a8f79b3017e2b8d8b
Author: Michael Paquier <michael@paquier.xyz>
Date: Mon Dec 2 11:18:56 2019 +0900
Refactor query cancellation code into src/fe_utils/
I've not dug into code itself, I just bisected it.
Cheers,
Jeff
explain (analyze) select * from pgbench_accounts \watch 1
It behaves as expected. But once I break out of the loop with ctrl-C, then
if I execute the same thing again it executes the command once, but shows
no output and doesn't loop. It seems like some flag is getting set with
ctrl-C, but then never gets reset.It was broken in this commit:
commit a4fd3aa719e8f97299dfcf1a8f79b3017e2b8d8b
Author: Michael Paquier <michael@paquier.xyz>
Date: Mon Dec 2 11:18:56 2019 +0900Refactor query cancellation code into src/fe_utils/
I've not dug into code itself, I just bisected it.
Thanks for the report. I'll look into it.
--
Fabien.
On Sat, Dec 14, 2019 at 12:09:51AM +0100, Fabien COELHO wrote:
explain (analyze) select * from pgbench_accounts \watch 1
It behaves as expected. But once I break out of the loop with ctrl-C, then
if I execute the same thing again it executes the command once, but shows
no output and doesn't loop. It seems like some flag is getting set with
ctrl-C, but then never gets reset.I've not dug into code itself, I just bisected it.
Thanks for the report. I'll look into it.
Looked at it already. And yes, I can see the difference. This comes
from the switch from cancel_pressed to CancelRequested in psql,
especially PSQLexecWatch() in this case. And actually, now that I
look at it, I think that we should simply get rid of cancel_pressed in
psql completely and replace it with CancelRequested. This also
removes the need of having cancel_pressed defined in print.c, which
was not really wanted originally. Attached is a patch which addresses
the issue for me, and cleans up the code while on it. Fabien, Jeff,
can you confirm please?
--
Michael
Attachments:
psql-cancel-fix.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
index f138d963d3..0324c48301 100644
--- a/src/include/fe_utils/print.h
+++ b/src/include/fe_utils/print.h
@@ -175,8 +175,6 @@ typedef struct printQueryOpt
} printQueryOpt;
-extern volatile bool cancel_pressed;
-
extern const printTextFormat pg_asciiformat;
extern const printTextFormat pg_asciiformat_old;
extern printTextFormat pg_utf8format; /* ideally would be const, but... */
diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index b9cd6a1752..7729a26ee2 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3,9 +3,10 @@
* Query-result printing support for frontend code
*
* This file used to be part of psql, but now it's separated out to allow
- * other frontend programs to use it. Because the printing code needs
- * access to the cancel_pressed flag as well as SIGPIPE trapping and
- * pager open/close functions, all that stuff came with it.
+ * other frontend programs to use it. The printing code relies on the
+ * cancellation logic in fe_utils with an access to the flag CancelRequested
+ * The SIGPIPE trapping and pager open/close functions came in with the
+ * original refactoring.
*
*
* Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
@@ -31,17 +32,17 @@
#endif
#include "catalog/pg_type_d.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/mbprint.h"
#include "fe_utils/print.h"
/*
* If the calling program doesn't have any mechanism for setting
- * cancel_pressed, it will have no effect.
+ * CancelRequested, note that query cancellation would have not effect.
*
- * Note: print.c's general strategy for when to check cancel_pressed is to do
+ * Note: print.c's general strategy for when to check CancelRequested is to do
* so at completion of each row of output.
*/
-volatile bool cancel_pressed = false;
static bool always_ignore_sigpipe = false;
@@ -371,7 +372,7 @@ print_unaligned_text(const printTableContent *cont, FILE *fout)
const char *const *ptr;
bool need_recordsep = false;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -406,7 +407,7 @@ print_unaligned_text(const printTableContent *cont, FILE *fout)
{
print_separator(cont->opt->recordSep, fout);
need_recordsep = false;
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
fputs(*ptr, fout);
@@ -422,7 +423,7 @@ print_unaligned_text(const printTableContent *cont, FILE *fout)
{
printTableFooter *footers = footers_with_default(cont);
- if (!opt_tuples_only && footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -462,7 +463,7 @@ print_unaligned_vertical(const printTableContent *cont, FILE *fout)
const char *const *ptr;
bool need_recordsep = false;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -487,7 +488,7 @@ print_unaligned_vertical(const printTableContent *cont, FILE *fout)
print_separator(cont->opt->recordSep, fout);
print_separator(cont->opt->recordSep, fout);
need_recordsep = false;
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
@@ -504,7 +505,7 @@ print_unaligned_vertical(const printTableContent *cont, FILE *fout)
if (cont->opt->stop_table)
{
/* print footers */
- if (!opt_tuples_only && cont->footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && cont->footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -614,7 +615,7 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
int output_columns = 0; /* Width of interactive console */
bool is_local_pager = false;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 2)
@@ -968,7 +969,7 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
{
bool more_lines;
- if (cancel_pressed)
+ if (CancelRequested)
break;
/*
@@ -1127,12 +1128,12 @@ print_aligned_text(const printTableContent *cont, FILE *fout, bool is_pager)
{
printTableFooter *footers = footers_with_default(cont);
- if (opt_border == 2 && !cancel_pressed)
+ if (opt_border == 2 && !CancelRequested)
_print_horizontal_line(col_count, width_wrap, opt_border,
PRINT_RULE_BOTTOM, format, fout);
/* print footers */
- if (footers && !opt_tuples_only && !cancel_pressed)
+ if (footers && !opt_tuples_only && !CancelRequested)
{
printTableFooter *f;
@@ -1249,7 +1250,7 @@ print_aligned_vertical(const printTableContent *cont,
dmultiline = false;
int output_columns = 0; /* Width of interactive console */
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 2)
@@ -1260,7 +1261,7 @@ print_aligned_vertical(const printTableContent *cont,
{
printTableFooter *footers = footers_with_default(cont);
- if (!opt_tuples_only && !cancel_pressed && footers)
+ if (!opt_tuples_only && !CancelRequested && footers)
{
printTableFooter *f;
@@ -1498,7 +1499,7 @@ print_aligned_vertical(const printTableContent *cont,
offset,
chars_to_output;
- if (cancel_pressed)
+ if (CancelRequested)
break;
if (i == 0)
@@ -1706,12 +1707,12 @@ print_aligned_vertical(const printTableContent *cont,
if (cont->opt->stop_table)
{
- if (opt_border == 2 && !cancel_pressed)
+ if (opt_border == 2 && !CancelRequested)
print_aligned_vertical_line(format, opt_border, 0, hwidth, dwidth,
PRINT_RULE_BOTTOM, fout);
/* print footers */
- if (!opt_tuples_only && cont->footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && cont->footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -1785,7 +1786,7 @@ print_csv_text(const printTableContent *cont, FILE *fout)
const char *const *ptr;
int i;
- if (cancel_pressed)
+ if (CancelRequested)
return;
/*
@@ -1828,7 +1829,7 @@ print_csv_vertical(const printTableContent *cont, FILE *fout)
/* print records */
for (i = 0, ptr = cont->cells; *ptr; i++, ptr++)
{
- if (cancel_pressed)
+ if (CancelRequested)
return;
/* print name of column */
@@ -1901,7 +1902,7 @@ print_html_text(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -1938,7 +1939,7 @@ print_html_text(const printTableContent *cont, FILE *fout)
{
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
fputs(" <tr valign=\"top\">\n", fout);
}
@@ -1963,7 +1964,7 @@ print_html_text(const printTableContent *cont, FILE *fout)
fputs("</table>\n", fout);
/* print footers */
- if (!opt_tuples_only && footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -1991,7 +1992,7 @@ print_html_vertical(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -2015,7 +2016,7 @@ print_html_vertical(const printTableContent *cont, FILE *fout)
{
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
if (!opt_tuples_only)
fprintf(fout,
@@ -2044,7 +2045,7 @@ print_html_vertical(const printTableContent *cont, FILE *fout)
fputs("</table>\n", fout);
/* print footers */
- if (!opt_tuples_only && cont->footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && cont->footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -2093,7 +2094,7 @@ print_asciidoc_text(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -2152,7 +2153,7 @@ print_asciidoc_text(const printTableContent *cont, FILE *fout)
{
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
@@ -2180,7 +2181,7 @@ print_asciidoc_text(const printTableContent *cont, FILE *fout)
printTableFooter *footers = footers_with_default(cont);
/* print footers */
- if (!opt_tuples_only && footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -2204,7 +2205,7 @@ print_asciidoc_vertical(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->start_table)
@@ -2243,7 +2244,7 @@ print_asciidoc_vertical(const printTableContent *cont, FILE *fout)
{
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
if (!opt_tuples_only)
fprintf(fout,
@@ -2270,7 +2271,7 @@ print_asciidoc_vertical(const printTableContent *cont, FILE *fout)
if (cont->opt->stop_table)
{
/* print footers */
- if (!opt_tuples_only && cont->footers != NULL && !cancel_pressed)
+ if (!opt_tuples_only && cont->footers != NULL && !CancelRequested)
{
printTableFooter *f;
@@ -2361,7 +2362,7 @@ print_latex_text(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 3)
@@ -2422,7 +2423,7 @@ print_latex_text(const printTableContent *cont, FILE *fout)
fputs(" \\\\\n", fout);
if (opt_border == 3)
fputs("\\hline\n", fout);
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
else
@@ -2439,7 +2440,7 @@ print_latex_text(const printTableContent *cont, FILE *fout)
fputs("\\end{tabular}\n\n\\noindent ", fout);
/* print footers */
- if (footers && !opt_tuples_only && !cancel_pressed)
+ if (footers && !opt_tuples_only && !CancelRequested)
{
printTableFooter *f;
@@ -2471,7 +2472,7 @@ print_latex_longtable_text(const printTableContent *cont, FILE *fout)
const char *last_opt_table_attr_char = NULL;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 3)
@@ -2607,7 +2608,7 @@ print_latex_longtable_text(const printTableContent *cont, FILE *fout)
if (opt_border == 3)
fputs(" \\hline\n", fout);
}
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
@@ -2625,7 +2626,7 @@ print_latex_vertical(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 2)
@@ -2658,7 +2659,7 @@ print_latex_vertical(const printTableContent *cont, FILE *fout)
/* new record */
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
if (!opt_tuples_only)
{
@@ -2688,7 +2689,7 @@ print_latex_vertical(const printTableContent *cont, FILE *fout)
fputs("\\end{tabular}\n\n\\noindent ", fout);
/* print footers */
- if (cont->footers && !opt_tuples_only && !cancel_pressed)
+ if (cont->footers && !opt_tuples_only && !CancelRequested)
{
printTableFooter *f;
@@ -2734,7 +2735,7 @@ print_troff_ms_text(const printTableContent *cont, FILE *fout)
unsigned int i;
const char *const *ptr;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 2)
@@ -2788,7 +2789,7 @@ print_troff_ms_text(const printTableContent *cont, FILE *fout)
if ((i + 1) % cont->ncolumns == 0)
{
fputc('\n', fout);
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
else
@@ -2802,7 +2803,7 @@ print_troff_ms_text(const printTableContent *cont, FILE *fout)
fputs(".TE\n.DS L\n", fout);
/* print footers */
- if (footers && !opt_tuples_only && !cancel_pressed)
+ if (footers && !opt_tuples_only && !CancelRequested)
{
printTableFooter *f;
@@ -2828,7 +2829,7 @@ print_troff_ms_vertical(const printTableContent *cont, FILE *fout)
const char *const *ptr;
unsigned short current_format = 0; /* 0=none, 1=header, 2=body */
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (opt_border > 2)
@@ -2864,7 +2865,7 @@ print_troff_ms_vertical(const printTableContent *cont, FILE *fout)
/* new record */
if (i % cont->ncolumns == 0)
{
- if (cancel_pressed)
+ if (CancelRequested)
break;
if (!opt_tuples_only)
{
@@ -2909,7 +2910,7 @@ print_troff_ms_vertical(const printTableContent *cont, FILE *fout)
fputs(".TE\n.DS L\n", fout);
/* print footers */
- if (cont->footers && !opt_tuples_only && !cancel_pressed)
+ if (cont->footers && !opt_tuples_only && !CancelRequested)
{
printTableFooter *f;
@@ -3052,7 +3053,7 @@ ClosePager(FILE *pagerpipe)
* pager quit as a result of the SIGINT, this message won't go
* anywhere ...
*/
- if (cancel_pressed)
+ if (CancelRequested)
fprintf(pagerpipe, _("Interrupted\n"));
pclose(pagerpipe);
@@ -3333,7 +3334,7 @@ printTable(const printTableContent *cont,
{
bool is_local_pager = false;
- if (cancel_pressed)
+ if (CancelRequested)
return;
if (cont->opt->format == PRINT_NOTHING)
@@ -3439,7 +3440,7 @@ printQuery(const PGresult *result, const printQueryOpt *opt,
r,
c;
- if (cancel_pressed)
+ if (CancelRequested)
return;
printTableInit(&cont, &opt->topt, opt->title,
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 48b6279403..f86d21850e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4508,7 +4508,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
long s = Min(i, 1000L);
pg_usleep(s * 1000L);
- if (cancel_pressed)
+ if (CancelRequested)
break;
i -= s;
}
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9c0d53689e..5f5908a5c2 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -257,9 +257,6 @@ psql_cancel_callback(void)
sigint_interrupt_enabled = false;
siglongjmp(sigint_interrupt_jmp, 1);
}
-
- /* else, set cancel flag to stop any long-running loops */
- CancelRequested = true;
}
#else
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index b3b9313b36..5ccd3a66a6 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -21,6 +21,7 @@
#include "common.h"
#include "common/logging.h"
#include "describe.h"
+#include "fe_utils/cancel.h"
#include "fe_utils/mbprint.h"
#include "fe_utils/print.h"
#include "fe_utils/string_utils.h"
@@ -1422,7 +1423,7 @@ describeTableDetails(const char *pattern, bool verbose, bool showSystem)
PQclear(res);
return false;
}
- if (cancel_pressed)
+ if (CancelRequested)
{
PQclear(res);
return false;
@@ -4748,7 +4749,7 @@ listTSParsersVerbose(const char *pattern)
return false;
}
- if (cancel_pressed)
+ if (CancelRequested)
{
PQclear(res);
return false;
@@ -5143,7 +5144,7 @@ listTSConfigsVerbose(const char *pattern)
return false;
}
- if (cancel_pressed)
+ if (CancelRequested)
{
PQclear(res);
return false;
@@ -5648,7 +5649,7 @@ listExtensionContents(const char *pattern)
PQclear(res);
return false;
}
- if (cancel_pressed)
+ if (CancelRequested)
{
PQclear(res);
return false;
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index f7b1b94599..83871de2d0 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -10,6 +10,7 @@
#include "command.h"
#include "common.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "input.h"
#include "mainloop.h"
#include "mb/pg_wchar.h"
@@ -88,7 +89,7 @@ MainLoop(FILE *source)
/*
* Clean up after a previous Control-C
*/
- if (cancel_pressed)
+ if (CancelRequested)
{
if (!pset.cur_cmd_interactive)
{
@@ -99,7 +100,7 @@ MainLoop(FILE *source)
break;
}
- cancel_pressed = false;
+ CancelRequested = false;
}
/*
@@ -121,7 +122,7 @@ MainLoop(FILE *source)
prompt_status = PROMPT_READY;
need_redisplay = false;
pset.stmt_lineno = 1;
- cancel_pressed = false;
+ CancelRequested = false;
if (pset.cur_cmd_interactive)
{
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index 3f7b64143f..875b06fe15 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -9,6 +9,7 @@
#include "common.h"
#include "common/logging.h"
+#include "fe_utils/cancel.h"
#include "variables.h"
/*
@@ -194,7 +195,7 @@ PrintVariables(VariableSpace space)
{
if (ptr->value)
printf("%s = '%s'\n", ptr->name, ptr->value);
- if (cancel_pressed)
+ if (CancelRequested)
break;
}
}
On Fri, Dec 13, 2019 at 9:45 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Dec 14, 2019 at 12:09:51AM +0100, Fabien COELHO wrote:
explain (analyze) select * from pgbench_accounts \watch 1
It behaves as expected. But once I break out of the loop with ctrl-C,
then
if I execute the same thing again it executes the command once, but
shows
no output and doesn't loop. It seems like some flag is getting set with
ctrl-C, but then never gets reset.I've not dug into code itself, I just bisected it.
Thanks for the report. I'll look into it.
Looked at it already. And yes, I can see the difference. This comes
from the switch from cancel_pressed to CancelRequested in psql,
especially PSQLexecWatch() in this case. And actually, now that I
look at it, I think that we should simply get rid of cancel_pressed in
psql completely and replace it with CancelRequested. This also
removes the need of having cancel_pressed defined in print.c, which
was not really wanted originally. Attached is a patch which addresses
the issue for me, and cleans up the code while on it. Fabien, Jeff,
can you confirm please?
--
Michael
This works for me.
Thanks,
Jeff
I've not dug into code itself, I just bisected it.
Thanks for the report. I'll look into it.
Looked at it already.
Ah, the magic of timezones!
And yes, I can see the difference. This comes from the switch from
cancel_pressed to CancelRequested in psql, especially PSQLexecWatch() in
this case. And actually, now that I look at it, I think that we should
simply get rid of cancel_pressed in psql completely and replace it with
CancelRequested. This also removes the need of having cancel_pressed
defined in print.c, which was not really wanted originally. Attached is
a patch which addresses the issue for me, and cleans up the code while
on it. Fabien, Jeff, can you confirm please?
Yep. Patch applies cleanly, compiles, works for me as well.
--
Fabien.
Michael Paquier <michael@paquier.xyz> writes:
Looked at it already. And yes, I can see the difference. This comes
from the switch from cancel_pressed to CancelRequested in psql,
especially PSQLexecWatch() in this case. And actually, now that I
look at it, I think that we should simply get rid of cancel_pressed in
psql completely and replace it with CancelRequested. This also
removes the need of having cancel_pressed defined in print.c, which
was not really wanted originally. Attached is a patch which addresses
the issue for me, and cleans up the code while on it. Fabien, Jeff,
can you confirm please?
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?
Also, perhaps I am missing something, but I do not see anyplace in the
current code base that ever *clears* CancelRequested. 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? Is it sane that
CancelRequested isn't declared volatile?
regards, tom lane
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.
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
Attachments:
psql-cancel-fix-v2.patchtext/x-diff; charset=us-asciiDownload
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;
}
On Mon, Dec 16, 2019 at 11:40:07AM +0900, Michael Paquier wrote:
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.
Merging both flags can actually prove to be tricky, as we have some
code paths involving --single-step where psql visibly assumes that a
cancellation pressed does not necessarily imply one that succeeds is
there is a cancellation object around (ExecQueryTuples, tuple printing
and \copy). So I have fixed the issue by making the code of psql
consistent with what we had before a4fd3aa. I think that it should be
actually possible to merge CancelRequested and cancel_pressed while
keeping the user-visible changes acceptable, and this requires a very
careful lookup.
--
Michael