On Thu, Mar 4, 2021 at 11:28 PM Pavel Stehule <pavel.steh...@gmail.com> wrote:
> čt 4. 3. 2021 v 7:37 odesílatel Pavel Stehule <pavel.steh...@gmail.com>
> napsal:
>> Here is a little bit updated patch - detection of end of any child process
>> cannot be used on WIN32.
Yeah, it's OK for me if this feature only works on Unix until the
right person for the job shows up with a patch. If there is no pspg
on Windows, how would we even know if it works?
> second version - after some thinking, I think the pager for \watch command
> should be controlled by option "pager" too. When the pager is disabled on
> psql level, then the pager will not be used for \watch too.
Makes sense.
+ long s = Min(i, 100L);
+
+ pg_usleep(1000L * s);
+
+ /*
+ * in this moment an pager process can be only one child of
+ * psql process. There cannot be other processes. So we can
+ * detect end of any child process for fast detection of
+ * pager process.
+ *
+ * This simple detection doesn't work on WIN32, because we
+ * don't know handle of process created by _popen function.
+ * Own implementation of _popen function based on CreateProcess
+ * looks like overkill in this moment.
+ */
+ if (pagerpipe)
+ {
+
+ int status;
+ pid_t pid;
+
+ pid = waitpid(-1, &status, WNOHANG);
+ if (pid)
+ break;
+ }
+
+#endif
+
if (cancel_pressed)
break;
I thought a bit about what we're really trying to achieve here. We
want to go to sleep until someone presses ^C, the pager exits, or a
certain time is reached. Here, we're waking up 10 times per second to
check for exited child processes. It works, but it does not spark
joy.
I thought about treating SIGCHLD the same way as we treat SIGINT: it
could use the siglongjmp() trick for a non-local exit from the signal
handler. (Hmm... I wonder why that pre-existing code bothers to check
cancel_pressed, considering it is running with
sigint_interrupt_enabled = true so it won't even set the flag.) It
feels clever, but then you'd still have the repeating short
pg_usleep() calls, for reasons described by commit 8c1a71d36f5. I do
not like sleep/poll loops. Think of the polar bears. I need to fix
all of these, as a carbon emission offset for cfbot.
Although there are probably several ways to do this efficiently, my
first thought was: let's try sigtimedwait()! If you block the signals
first, you have a race-free way to wait for SIGINT (^C), SIGCHLD
(pager exited) or a timeout you can specify. I coded that up and it
worked pretty nicely, but then I tried it on macOS too and it didn't
compile -- Apple didn't implement that. Blah.
Next I tried sigwait(). That's already used in our tree, so it should
be OK. At first I thought that using SIGALRM to wake it up would be a
bit too ugly and I was going to give up, but then I realised that an
interval timer (one that automatically fires every X seconds) is
exactly what we want here, and we can set it up just once at the start
of do_watch() and cancel it at the end of do_watch(). With the
attached patch you get exactly one sigwait() syscall of the correct
duration per \watch cycle.
Thoughts? I put my changes into a separate patch for clarity, but
they need some more tidying up.
I'll look at the documentation next.
From e2d6d4a7ee33defbc9eb33a8bec53bac57871922 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 20 Mar 2021 21:54:27 +1300
Subject: [PATCH v3 1/2] Support PSQL_WATCH_PAGER for psql's \watch command.
---
doc/src/sgml/ref/psql-ref.sgml | 27 ++++++++++++
src/bin/psql/command.c | 81 +++++++++++++++++++++++++++++++---
src/bin/psql/common.c | 11 +++--
src/bin/psql/common.h | 2 +-
src/bin/psql/help.c | 4 +-
5 files changed, 112 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 01ec9b8b0a..5f594e5e0e 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2993,6 +2993,11 @@ lo_import 152801
(such as <filename>more</filename>) is used.
</para>
+ <para>
+ When the ennvironment variable <envar>PSQL_WATCH_PAGER</envar> is set, then
+ the output of repeated execution of query is piped to the specified program.
+ </para>
+
<para>
When the <literal>pager</literal> option is <literal>off</literal>, the pager
program is not used. When the <literal>pager</literal> option is
@@ -3004,6 +3009,13 @@ lo_import 152801
without a <replaceable class="parameter">value</replaceable>
toggles pager use on and off.
</para>
+
+ <para>
+ When an command <command>\watch</command> is executed, and environment
+ variable <envar>PSQL_WATCH_PAGER</envar> is defined, but the value of
+ the option <literal>pager</literal> is <literal>off</literal>, then the
+ pager is not used.
+ </para>
</listitem>
</varlistentry>
@@ -4663,6 +4675,21 @@ PSQL_EDITOR_LINENUMBER_ARG='--line '
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><envar>PSQL_WATCH_PAGER</envar></term>
+
+ <listitem>
+ <para>
+ When an statement is evaluated repeatedly because <command>\watch</command>
+ was used, then an pager is not used. This behaviour can be changed by
+ setting <envar>PSQL_WATCH_PAGER</envar> to pager with necessary options.
+ Currently only <literal>pspg</literal> pager (version 3.0+) supports this
+ functionality (with an option <literal>--stream</literal>).
+ </para>
+
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><envar>PSQLRC</envar></term>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 838f74bbbb..9037afbd6f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -13,6 +13,7 @@
#include <utime.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
+#include <sys/wait.h> /* for waitpid() */
#include <fcntl.h> /* open() flags */
#include <unistd.h> /* for geteuid(), getpid(), stat() */
#else
@@ -4787,6 +4788,8 @@ do_watch(PQExpBuffer query_buf, double sleep)
const char *strftime_fmt;
const char *user_title;
char *title;
+ const char *pagerprog = NULL;
+ FILE *pagerpipe = NULL;
int title_len;
int res = 0;
@@ -4796,6 +4799,25 @@ do_watch(PQExpBuffer query_buf, double sleep)
return false;
}
+ /*
+ * For usual queries, the pager can be used always, or
+ * newer, or when then content is larger than screen. In this case,
+ * the decision based on then content size has not sense, because the
+ * content can be different any time, but pager (in this case) is
+ * used longer time. So we use pager if the variable PSQL_WATCH_PAGER
+ * is defined and if pager is not disabled.
+ */
+ pagerprog = getenv("PSQL_WATCH_PAGER");
+ if (pagerprog && myopt.topt.pager)
+ {
+ disable_sigpipe_trap();
+ pagerpipe = popen(pagerprog, "w");
+
+ if (!pagerpipe)
+ /*silently proceed without pager */
+ restore_sigpipe_trap();
+ }
+
/*
* Choose format for timestamps. We might eventually make this a \pset
* option. In the meantime, using a variable for the format suppresses
@@ -4804,10 +4826,12 @@ do_watch(PQExpBuffer query_buf, double sleep)
strftime_fmt = "%c";
/*
- * Set up rendering options, in particular, disable the pager, because
- * nobody wants to be prompted while watching the output of 'watch'.
+ * Set up rendering options, in particular, disable the pager, when
+ * there an pipe to pager is not available.
*/
- myopt.topt.pager = 0;
+ if (!pagerpipe)
+ myopt.topt.pager = 0;
+
/*
* If there's a title in the user configuration, make sure we have room
@@ -4841,7 +4865,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
myopt.title = title;
/* Run the query and print out the results */
- res = PSQLexecWatch(query_buf->data, &myopt);
+ res = PSQLexecWatch(query_buf->data, &myopt, pagerpipe);
/*
* PSQLexecWatch handles the case where we can no longer repeat the
@@ -4850,6 +4874,9 @@ do_watch(PQExpBuffer query_buf, double sleep)
if (res <= 0)
break;
+ if (pagerpipe && ferror(pagerpipe))
+ break;
+
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
* through the loop since it's conceivable something inside
@@ -4860,23 +4887,63 @@ do_watch(PQExpBuffer query_buf, double sleep)
/*
* Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s)
- * since pg_usleep isn't interruptible on some platforms.
+ * query again. Break the sleep into short intervals (at most 100ms)
+ * since pg_usleep isn't interruptible on some platforms. The overhead
+ * of 100ms interval is compromise between overhead and ergonomentry
+ * (we don't want to wait long time after pager was ended).
*/
sigint_interrupt_enabled = true;
i = sleep_ms;
while (i > 0)
{
+#ifdef WIN32
long s = Min(i, 1000L);
- pg_usleep(s * 1000L);
+ pg_usleep(1000L * s);
+
+#else
+ long s = Min(i, 100L);
+
+ pg_usleep(1000L * s);
+
+ /*
+ * in this moment an pager process can be only one child of
+ * psql process. There cannot be other processes. So we can
+ * detect end of any child process for fast detection of
+ * pager process.
+ *
+ * This simple detection doesn't work on WIN32, because we
+ * don't know handle of process created by _popen function.
+ * Own implementation of _popen function based on CreateProcess
+ * looks like overkill in this moment.
+ */
+ if (pagerpipe)
+ {
+
+ int status;
+ pid_t pid;
+
+ pid = waitpid(-1, &status, WNOHANG);
+ if (pid)
+ break;
+ }
+
+#endif
+
if (cancel_pressed)
break;
+
i -= s;
}
sigint_interrupt_enabled = false;
}
+ if (pagerpipe)
+ {
+ pclose(pagerpipe);
+ restore_sigpipe_trap();
+ }
+
pg_free(title);
return (res >= 0);
}
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 7a95465111..b2811bbc60 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -592,12 +592,13 @@ PSQLexec(const char *query)
* e.g., because of the interrupt, -1 on error.
*/
int
-PSQLexecWatch(const char *query, const printQueryOpt *opt)
+PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout)
{
PGresult *res;
double elapsed_msec = 0;
instr_time before;
instr_time after;
+ FILE *fout;
if (!pset.db)
{
@@ -638,14 +639,16 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt)
return 0;
}
+ fout = printQueryFout ? printQueryFout : pset.queryFout;
+
switch (PQresultStatus(res))
{
case PGRES_TUPLES_OK:
- printQuery(res, opt, pset.queryFout, false, pset.logfile);
+ printQuery(res, opt, fout, false, pset.logfile);
break;
case PGRES_COMMAND_OK:
- fprintf(pset.queryFout, "%s\n%s\n\n", opt->title, PQcmdStatus(res));
+ fprintf(fout, "%s\n%s\n\n", opt->title, PQcmdStatus(res));
break;
case PGRES_EMPTY_QUERY:
@@ -668,7 +671,7 @@ PSQLexecWatch(const char *query, const printQueryOpt *opt)
PQclear(res);
- fflush(pset.queryFout);
+ fflush(fout);
/* Possible microtiming output */
if (pset.timing)
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index 041b2ac068..d8538a4e06 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -29,7 +29,7 @@ extern sigjmp_buf sigint_interrupt_jmp;
extern void psql_setup_cancel_handler(void);
extern PGresult *PSQLexec(const char *query);
-extern int PSQLexecWatch(const char *query, const printQueryOpt *opt);
+extern int PSQLexecWatch(const char *query, const printQueryOpt *opt, FILE *printQueryFout);
extern bool SendQuery(const char *query);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 99a59470c5..f465015dc1 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -345,7 +345,7 @@ helpVariables(unsigned short int pager)
* Windows builds currently print one more line than non-Windows builds.
* Using the larger number is fine.
*/
- output = PageOutput(158, pager ? &(pset.popt.topt) : NULL);
+ output = PageOutput(160, pager ? &(pset.popt.topt) : NULL);
fprintf(output, _("List of specially treated variables\n\n"));
@@ -503,6 +503,8 @@ helpVariables(unsigned short int pager)
" alternative location for the command history file\n"));
fprintf(output, _(" PSQL_PAGER, PAGER\n"
" name of external pager program\n"));
+ fprintf(output, _(" PSQL_WATCH_PAGER\n"
+ " name of external pager program used for watch mode\n"));
fprintf(output, _(" PSQLRC\n"
" alternative location for the user's .psqlrc file\n"));
fprintf(output, _(" SHELL\n"
--
2.30.1
From 5587f4b3c04c94feaf056478efbd9e46d40674da Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sun, 21 Mar 2021 00:51:03 +1300
Subject: [PATCH v3 2/2] fixup: Let's try sigwait()
---
src/bin/psql/command.c | 105 +++++++++++++++++++++++++++--------------
src/bin/psql/common.h | 4 ++
src/bin/psql/startup.c | 14 ++++++
3 files changed, 88 insertions(+), 35 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9037afbd6f..efede160ca 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -13,7 +13,7 @@
#include <utime.h>
#ifndef WIN32
#include <sys/stat.h> /* for stat() */
-#include <sys/wait.h> /* for waitpid() */
+#include <sys/time.h> /* for setitimer() */
#include <fcntl.h> /* open() flags */
#include <unistd.h> /* for geteuid(), getpid(), stat() */
#else
@@ -4792,6 +4792,11 @@ do_watch(PQExpBuffer query_buf, double sleep)
FILE *pagerpipe = NULL;
int title_len;
int res = 0;
+#ifndef WIN32
+ sigset_t sigset;
+ struct itimerval interval;
+ bool done = false;
+#endif
if (!query_buf || query_buf->len <= 0)
{
@@ -4799,6 +4804,32 @@ do_watch(PQExpBuffer query_buf, double sleep)
return false;
}
+#ifndef WIN32
+ /*
+ * Block the signals we're interested in before we start the watch pager,
+ * if configured, to avoid races. sigwait() will receive them.
+ */
+ sigemptyset(&sigset);
+ sigaddset(&sigset, SIGINT);
+ sigaddset(&sigset, SIGCHLD);
+ sigaddset(&sigset, SIGALRM);
+ sigprocmask(SIG_BLOCK, &sigset, NULL);
+
+ /*
+ * Set a timer to interrupt sigwait() at the right intervals to run the
+ * query again. We can't use the obvious sigtimedwait() instead, because
+ * macOS hasn't got it.
+ */
+ interval.it_value.tv_sec = sleep_ms / 1000;
+ interval.it_value.tv_usec = (sleep_ms % 1000) * 1000;
+ interval.it_interval = interval.it_value;
+ if (setitimer(ITIMER_REAL, &interval, NULL) < 0)
+ {
+ pg_log_error("could not set timer: %m");
+ done = true;
+ }
+#endif
+
/*
* For usual queries, the pager can be used always, or
* newer, or when then content is larger than screen. In this case,
@@ -4846,7 +4877,6 @@ do_watch(PQExpBuffer query_buf, double sleep)
{
time_t timer;
char timebuf[128];
- long i;
/*
* Prepare title for output. Note that we intentionally include a
@@ -4877,6 +4907,7 @@ do_watch(PQExpBuffer query_buf, double sleep)
if (pagerpipe && ferror(pagerpipe))
break;
+#ifdef WIN32
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
* through the loop since it's conceivable something inside
@@ -4893,49 +4924,45 @@ do_watch(PQExpBuffer query_buf, double sleep)
* (we don't want to wait long time after pager was ended).
*/
sigint_interrupt_enabled = true;
- i = sleep_ms;
- while (i > 0)
+ for (int i = sleep_ms; i > 0;)
{
-#ifdef WIN32
- long s = Min(i, 1000L);
-
- pg_usleep(1000L * s);
-
-#else
- long s = Min(i, 100L);
+ int s = Min(i, 1000L);
pg_usleep(1000L * s);
- /*
- * in this moment an pager process can be only one child of
- * psql process. There cannot be other processes. So we can
- * detect end of any child process for fast detection of
- * pager process.
- *
- * This simple detection doesn't work on WIN32, because we
- * don't know handle of process created by _popen function.
- * Own implementation of _popen function based on CreateProcess
- * looks like overkill in this moment.
- */
- if (pagerpipe)
- {
-
- int status;
- pid_t pid;
-
- pid = waitpid(-1, &status, WNOHANG);
- if (pid)
- break;
- }
-
-#endif
-
if (cancel_pressed)
break;
i -= s;
}
sigint_interrupt_enabled = false;
+#else
+ /* Wait for SIGINT, SIGCHLD or SIGALRM. */
+ while (!done)
+ {
+ int signal_received;
+
+ if (sigwait(&sigset, &signal_received) < 0)
+ {
+ /* Some other signal arrived? */
+ if (errno == EINTR)
+ continue;
+ else
+ {
+ pg_log_error("could not wait for signals: %m");
+ done = true;
+ break;
+ }
+ }
+ /* On ^C or pager exit, it's time to stop running the query. */
+ if (signal_received == SIGINT || signal_received == SIGCHLD)
+ done = true;
+ /* Otherwise, we must have SIGALRM. Time to run the query again. */
+ break;
+ }
+ if (done)
+ break;
+#endif
}
if (pagerpipe)
@@ -4944,6 +4971,14 @@ do_watch(PQExpBuffer query_buf, double sleep)
restore_sigpipe_trap();
}
+#ifndef WIN32
+ /* Disable the interval timer. */
+ memset(&interval, 0, sizeof(interval));
+ setitimer(ITIMER_REAL, &interval, NULL);
+ /* Unblock SIGINT, SIGCHLD and SIGALRM. */
+ sigprocmask(SIG_UNBLOCK, &sigset, NULL);
+#endif
+
pg_free(title);
return (res >= 0);
}
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index d8538a4e06..34eb9b4653 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -9,11 +9,15 @@
#define COMMON_H
#include <setjmp.h>
+#include <signal.h>
#include "fe_utils/print.h"
#include "fe_utils/psqlscan.h"
#include "libpq-fe.h"
+extern volatile sig_atomic_t received_sigchld;
+extern void handle_sigchld(SIGNAL_ARGS);
+
extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
extern bool setQFout(const char *fname);
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 110906a4e9..369b34fd84 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -110,6 +110,11 @@ log_locus_callback(const char **filename, uint64 *lineno)
}
}
+static void
+empty_signal_handler(SIGNAL_ARGS)
+{
+}
+
/*
*
* main
@@ -302,6 +307,15 @@ main(int argc, char *argv[])
psql_setup_cancel_handler();
+ /*
+ * do_watch() needs signal handlers installed (otherwise sigwait() will
+ * filter them out on some platforms), but doesn't need them to do
+ * anything, and they shouldn't ever run (unless perhaps a stray SIGALRM
+ * arrives due to a race when do_watch() cancels an itimer).
+ */
+ pqsignal(SIGCHLD, empty_signal_handler);
+ pqsignal(SIGALRM, empty_signal_handler);
+
PQsetNoticeProcessor(pset.db, NoticeProcessor, NULL);
SyncVariables();
--
2.30.1
