On Thu, Apr 1, 2021 at 10:16 PM tsunakawa.ta...@fujitsu.com
<tsunakawa.ta...@fujitsu.com> wrote:
> From: Thomas Munro <thomas.mu...@gmail.com>
> > I changed my mind. Let's commit the pleasingly simple Linux-only feature
> > for
> > now, and extend to it to send some kind of no-op message in a later release.
> > So this is the version I'd like to go with.
> > Objections?
>
> +1, as some of our users experienced the problem that the server kept
> processing (IIRC, a buggy PL/pgSQL procedure that loops indefinitely) after
> they killed the client.
Cool. Yeah, I have seen a few variants of that, and several other
complaints on the lists.
> TBH, Linux and Windows will be sufficient. But I'm for providing a good
> feature on a specific OS first.
I discovered that at least one other OS has adopted POLLRDHUP, so I
changed the language to something slightly more general:
+ <para>
+ This option is currently available only on systems that support the
+ non-standard <symbol>POLLRDHUP</symbol> extension to the
+ <symbol>poll</symbol> system call, including Linux.
+ </para>
It seems like it must be quite easy for an OS to implement, since the
TCP stack surely has the information... it's just an API problem.
Hopefully that means that there aren't OSes that define the macro but
don't work the same way. (I read somewhere that the POSIX compliance
test suite explicitly tests this half-shutdown case and fails any OS
that returns SIGHUP "prematurely". Boo.)
> (1)
> + rc = poll(&pollfd, 1, 0);
> + if (rc < 0)
> + {
> + elog(COMMERROR, "could not poll socket: %m");
> + return false;
>
> I think it's customary to use ereport() and errcode_for_socket_access().
Fixed.
> (2)
> pq_check_connection()
>
> Following PostmasterIsAlive(), maybe it's better to name it as
> pq_connection_is_alive() pq_client_is_alive(), or pq_frontend_is_alive() like
> the pqcomm.c's head comment uses the word frontend?
I think it's OK, because it matches the name of the GUC. I'm more
concerned about the name of the GUC. Will we still be happy with this
name if a future releases sends a heartbeat message? I think that is
still OK, so I'm happy with these names for now, but if someone has a
better name, please speak up very soon.
> (3)
> #include <limits.h>
> +#include <poll.h>
> #ifndef WIN32
> #include <sys/mman.h>
> #endif
>
> poll.h should be included between #ifndef WIN32 and #endif?
Oops, I forgot to wrap that in #ifdef HAVE_POLL_H while moving stuff
around. Fixed.
> (4)
> I think the new GUC works for walsender as well. If so, how do we explain
> the relationship between the new GUC and wal_receiver_timeout and recommend
> the settings of them?
No, it only works while executing a query. (Is there something in
logical decoding, perhaps, that I have failed to consider?)
PS The "from" headers in emails received from Fujitsu seems to have
the names stripped, somewhere in the tubes of the internet. I see the
full version when people from Fujitsu quote other people from Fujitsu.
I copied one of those into the commit message, complete with its
magnificent kanji characters (perhaps these are the cause of the
filtering?), and I hope that's OK with you.
From 7c045e992469c88656655e2b460fb8622d7ac790 Mon Sep 17 00:00:00 2001
From: Maksim Milyutin <milyuti...@gmail.com>
Date: Fri, 26 Mar 2021 10:18:30 +0300
Subject: [PATCH v11] Detect POLLHUP/POLLRDHUP while running queries.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Provide a new GUC check_client_connection_interval that can be used to
check whether the client connection has gone away, while running very
long queries. It is disabled by default.
For now this uses a non-standard Linux extension. POLLRDHUP is not
defined by POSIX, and other OSes don't have a reliable way to know if a
connection was shut down without actually trying to read or write. In
future we might extend this to other OSes by trying to send a
no-op/heartbeat message, but that may require protocol changes.
Author: Sergey Cherkashin <s.cherkas...@postgrespro.ru>
Author: Thomas Munro <thomas.mu...@gmail.com>
Reviewed-by: Thomas Munro <thomas.mu...@gmail.com>
Reviewed-by: Tatsuo Ishii <is...@sraoss.co.jp>
Reviewed-by: Konstantin Knizhnik <k.knizh...@postgrespro.ru>
Reviewed-by: Zhihong Yu <z...@yugabyte.com>
Reviewed-by: Andres Freund <and...@anarazel.de>
Reviewed-by: Maksim Milyutin <milyuti...@gmail.com>
Reviewed-by: Tsunakawa, Takayuki/綱川 貴之 <tsunakawa.ta...@fujitsu.com>
Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> (much earlier version)
Discussion: https://postgr.es/m/77def86b27e41f0efcba411460e929ae%40postgrespro.ru
---
doc/src/sgml/config.sgml | 37 +++++++++++++++++
src/backend/libpq/pqcomm.c | 40 +++++++++++++++++++
src/backend/tcop/postgres.c | 32 +++++++++++++++
src/backend/utils/init/globals.c | 1 +
src/backend/utils/init/postinit.c | 10 +++++
src/backend/utils/misc/guc.c | 29 ++++++++++++++
src/backend/utils/misc/postgresql.conf.sample | 3 ++
src/include/libpq/libpq.h | 1 +
src/include/miscadmin.h | 1 +
src/include/tcop/tcopprot.h | 1 +
src/include/utils/timeout.h | 1 +
11 files changed, 156 insertions(+)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 9d87b5097a..0c9128a55d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -998,6 +998,43 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-client-connection-check-interval" xreflabel="client_connection_check_interval">
+ <term><varname>client_connection_check_interval</varname> (<type>integer</type>)
+ <indexterm>
+ <primary><varname>client_connection_check_interval</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Sets the time interval between optional checks that the client is still
+ connected, while running queries. The check is performed by polling
+ the socket, and allows long running queries to be aborted sooner if
+ the kernel reports that the connection is closed.
+ </para>
+ <para>
+ This option is currently available only on systems that support the
+ non-standard <symbol>POLLRDHUP</symbol> extension to the
+ <symbol>poll</symbol> system call, including Linux.
+ </para>
+ <para>
+ If the value is specified without units, it is taken as milliseconds.
+ The default value is <literal>0</literal>, which disables connection
+ checks. Without connection checks, the server will detect the loss of
+ the connection only at the next interaction with the socket, when it
+ waits for, receives or sends data.
+ </para>
+ <para>
+ For the kernel itself to detect lost TCP connections reliably and within
+ a known timeframe in all scenarios including network failure, it may
+ also be necessary to adjust the TCP keepalive settings of the operating
+ system, or the <xref linkend="guc-tcp-keepalives-idle"/>,
+ <xref linkend="guc-tcp-keepalives-interval"/> and
+ <xref linkend="guc-tcp-keepalives-count"/> settings of
+ <productname>PostgreSQL</productname>.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 4c7b1e7bfd..697c8c79af 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -54,6 +54,9 @@
*/
#include "postgres.h"
+#ifdef HAVE_POLL_H
+#include <poll.h>
+#endif
#include <signal.h>
#include <fcntl.h>
#include <grp.h>
@@ -1921,3 +1924,40 @@ pq_settcpusertimeout(int timeout, Port *port)
return STATUS_OK;
}
+
+/*
+ * Check if the kernel thinks the client is still connected.
+ */
+bool
+pq_check_connection(void)
+{
+#if defined(POLLRDHUP)
+ /*
+ * POLLRDHUP is a Linux extension to poll(2) to detect sockets closed by
+ * the other end. We don't have a portable way to do that without
+ * actually trying to read or write data on other systems. We don't want
+ * to read because that would be confused by pipelined queries and COPY
+ * data. Perhaps in future we'll try to write a heartbeat message instead.
+ */
+ struct pollfd pollfd;
+ int rc;
+
+ pollfd.fd = MyProcPort->sock;
+ pollfd.events = POLLOUT | POLLIN | POLLRDHUP;
+ pollfd.revents = 0;
+
+ rc = poll(&pollfd, 1, 0);
+
+ if (rc < 0)
+ {
+ ereport(COMMERROR,
+ (errcode_for_socket_access(),
+ errmsg("could not poll socket: %m")));
+ return false;
+ }
+ else if (rc == 1 && (pollfd.revents & (POLLHUP | POLLRDHUP)))
+ return false;
+#endif
+
+ return true;
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 2b1b68109f..ad351e2fd1 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -102,6 +102,9 @@ int max_stack_depth = 100;
/* wait N seconds to allow attach from a debugger */
int PostAuthDelay = 0;
+/* Time between checks that the client is still connected. */
+int client_connection_check_interval = 0;
+
/* ----------------
* private typedefs etc
* ----------------
@@ -2671,6 +2674,14 @@ start_xact_command(void)
* not desired, the timeout has to be disabled explicitly.
*/
enable_statement_timeout();
+
+ /* Start timeout for checking if the client has gone away if necessary. */
+ if (client_connection_check_interval > 0 &&
+ IsUnderPostmaster &&
+ MyProcPort &&
+ !get_timeout_active(CLIENT_CONNECTION_CHECK_TIMEOUT))
+ enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
+ client_connection_check_interval);
}
static void
@@ -3149,6 +3160,27 @@ ProcessInterrupts(void)
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating connection due to administrator command")));
}
+
+ if (CheckClientConnectionPending)
+ {
+ CheckClientConnectionPending = false;
+
+ /*
+ * Check for lost connection and re-arm, if still configured, but not
+ * if we've arrived back at DoingCommandRead state. We don't want to
+ * wake up idle sessions, and they already know how to detect lost
+ * connections.
+ */
+ if (!DoingCommandRead && client_connection_check_interval > 0)
+ {
+ if (!pq_check_connection())
+ ClientConnectionLost = true;
+ else
+ enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
+ client_connection_check_interval);
+ }
+ }
+
if (ClientConnectionLost)
{
QueryCancelPending = false; /* lost connection trumps QueryCancel */
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 73e0a672ae..a9f0fc3017 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -30,6 +30,7 @@ ProtocolVersion FrontendProtocol;
volatile sig_atomic_t InterruptPending = false;
volatile sig_atomic_t QueryCancelPending = false;
volatile sig_atomic_t ProcDiePending = false;
+volatile sig_atomic_t CheckClientConnectionPending = false;
volatile sig_atomic_t ClientConnectionLost = false;
volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false;
volatile sig_atomic_t IdleSessionTimeoutPending = false;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 7abeccb536..a3ec358538 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -73,6 +73,7 @@ static void StatementTimeoutHandler(void);
static void LockTimeoutHandler(void);
static void IdleInTransactionSessionTimeoutHandler(void);
static void IdleSessionTimeoutHandler(void);
+static void ClientCheckTimeoutHandler(void);
static bool ThereIsAtLeastOneRole(void);
static void process_startup_options(Port *port, bool am_superuser);
static void process_settings(Oid databaseid, Oid roleid);
@@ -620,6 +621,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IdleInTransactionSessionTimeoutHandler);
RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
+ RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
}
/*
@@ -1242,6 +1244,14 @@ IdleSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+ClientCheckTimeoutHandler(void)
+{
+ CheckClientConnectionPending = true;
+ InterruptPending = true;
+ SetLatch(MyLatch);
+}
+
/*
* Returns true if at least one role is defined in this database cluster.
*/
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 584daffc8a..60a9c7a2a0 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -20,6 +20,9 @@
#include <float.h>
#include <math.h>
#include <limits.h>
+#ifdef HAVE_POLL_H
+#include <poll.h>
+#endif
#ifndef WIN32
#include <sys/mman.h>
#endif
@@ -204,6 +207,7 @@ static bool check_autovacuum_work_mem(int *newval, void **extra, GucSource sourc
static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source);
static bool check_maintenance_io_concurrency(int *newval, void **extra, GucSource source);
static bool check_huge_page_size(int *newval, void **extra, GucSource source);
+static bool check_client_connection_check_interval(int *newval, void **extra, GucSource source);
static void assign_pgstat_temp_directory(const char *newval, void *extra);
static bool check_application_name(char **newval, void **extra, GucSource source);
static void assign_application_name(const char *newval, void *extra);
@@ -3501,6 +3505,17 @@ static struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"client_connection_check_interval", PGC_USERSET, CLIENT_CONN_OTHER,
+ gettext_noop("Sets the time interval between checks for disconnection while running queries."),
+ NULL,
+ GUC_UNIT_MS
+ },
+ &client_connection_check_interval,
+ 0, 0, INT_MAX,
+ check_client_connection_check_interval, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
@@ -11980,6 +11995,20 @@ check_huge_page_size(int *newval, void **extra, GucSource source)
return true;
}
+static bool
+check_client_connection_check_interval(int *newval, void **extra, GucSource source)
+{
+#ifndef POLLRDHUP
+ /* Linux only, for now. See pq_check_connection(). */
+ if (*newval != 0)
+ {
+ GUC_check_errdetail("client_connection_check_interval must be set to 0 on platforms that lack POLLRDHUP.");
+ return false;
+ }
+#endif
+ return true;
+}
+
static void
assign_pgstat_temp_directory(const char *newval, void *extra)
{
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 30cfddac1f..39da7cc942 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -719,6 +719,9 @@
#dynamic_library_path = '$libdir'
+#client_connection_check_interval = 0 # time between checks for client
+ # disconnection while running queries;
+ # 0 for never
#------------------------------------------------------------------------------
# LOCK MANAGEMENT
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index b20deeb555..3ebbc8d665 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -71,6 +71,7 @@ extern int pq_getbyte(void);
extern int pq_peekbyte(void);
extern int pq_getbyte_if_available(unsigned char *c);
extern int pq_putmessage_v2(char msgtype, const char *s, size_t len);
+extern bool pq_check_connection(void);
/*
* prototypes for functions in be-secure.c
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 013850ac28..6f8251e0b0 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -85,6 +85,7 @@ extern PGDLLIMPORT volatile sig_atomic_t IdleInTransactionSessionTimeoutPending;
extern PGDLLIMPORT volatile sig_atomic_t IdleSessionTimeoutPending;
extern PGDLLIMPORT volatile sig_atomic_t ProcSignalBarrierPending;
+extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending;
extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost;
/* these are marked volatile because they are examined by signal handlers: */
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index e5472100a4..241e7c9961 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -29,6 +29,7 @@ extern CommandDest whereToSendOutput;
extern PGDLLIMPORT const char *debug_query_string;
extern int max_stack_depth;
extern int PostAuthDelay;
+extern int client_connection_check_interval;
/* GUC-configurable parameters */
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index ecb2a366a5..93e6a691b3 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -32,6 +32,7 @@ typedef enum TimeoutId
STANDBY_LOCK_TIMEOUT,
IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IDLE_SESSION_TIMEOUT,
+ CLIENT_CONNECTION_CHECK_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,
/* Maximum number of timeout reasons */
--
2.30.1
