From 5587f4b3c04c94feaf056478efbd9e46d40674da Mon Sep 17 00:00:00 2001 From: Thomas Munro 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 #ifndef WIN32 #include /* for stat() */ -#include /* for waitpid() */ +#include /* for setitimer() */ #include /* open() flags */ #include /* 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 +#include #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