On Mon, Mar 1, 2021 at 6:18 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> I've done a quick rebase of this the patch and added it to the
> commitfest. No other changes. Several things were mentioned earlier
> that still need to be tidied up.
Rebased again due to bitrot. This time I did some actual work:
1. I didn't like the way it was rearming the timer *in the timer
handler*; I think it should be done in the CFI(), and only if it
determines that you're still running a query (otherwise you'll get
periodic wakeups while you're idle between quieries, which is bad for
the arctic ice cap; we already handle client going away efficiently
between queries with WaitEventSet socket readiness).
2. The timer handler surely has to set the latch to close a race (cf.
other similar handlers; between the CFI() and the beginning of the
sleep, you could handle the signal, set the flag, and then go to sleep
for 100 years).
3. The test might as well use pg_sleep() instead of doing a plpgsql
busy loop of SELECT queries.
4. I prefer the name CLIENT_CONNECTION_CHECK_TIMEOUT instead of
SKIP_CLIENT_CHECK_TIMEOUT; let's make up only one new name for a
concept instead of two.
5. Miniscule doc change.
I put these into a separate patch for ease of review. I don't claim
this is ready -- still needs more testing etc -- but it seems to be
generating the right system calls at the right times now.
From 8962b1ac40cf63c90af3e65dc15d554d764483dc Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Mon, 1 Mar 2021 18:08:23 +1300
Subject: [PATCH v4 1/2] Detect dropped connections while running queries.
Provide a new optional GUC that can be used to check whether the client
connection has gone away periodically while running very long queries.
Author: Sergey Cherkashin <s.cherkas...@postgrespro.ru>
Author: <your name here>
Discussion: https://postgr.es/m/77def86b27e41f0efcba411460e929ae%40postgrespro.ru
---
doc/src/sgml/config.sgml | 20 ++++
src/backend/libpq/pqcomm.c | 31 +++++
src/backend/tcop/postgres.c | 5 +
src/backend/utils/init/globals.c | 1 +
src/backend/utils/init/postinit.c | 13 +++
src/backend/utils/misc/guc.c | 10 ++
src/backend/utils/misc/postgresql.conf.sample | 3 +
src/include/libpq/libpq.h | 2 +
src/include/miscadmin.h | 3 +-
src/include/utils/timeout.h | 1 +
src/test/modules/Makefile | 1 +
src/test/modules/connection/Makefile | 16 +++
.../connection/t/001_close_connection.pl | 107 ++++++++++++++++++
13 files changed, 212 insertions(+), 1 deletion(-)
create mode 100644 src/test/modules/connection/Makefile
create mode 100644 src/test/modules/connection/t/001_close_connection.pl
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 967de73596..cadf6a9e7d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9231,6 +9231,26 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</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 a time interval, in milliseconds, between periodic
+ verification of client-server connection during query execution.
+ If the client aborts the connection, the query is terminated.
+ </para>
+ <para>
+ Default value is <literal>zero</literal> - it disables connection
+ checks, so the backend will detect client disconnection only when trying
+ to send a response to the query.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
</sect1>
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 4c7b1e7bfd..9f33138105 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -104,6 +104,7 @@
*/
int Unix_socket_permissions;
char *Unix_socket_group;
+int client_connection_check_interval;
/* Where the Unix socket files are (list of palloc'd strings) */
static List *sock_paths = NIL;
@@ -1921,3 +1922,33 @@ pq_settcpusertimeout(int timeout, Port *port)
return STATUS_OK;
}
+
+/* --------------------------------
+ * pq_check_client_connection - check if client connected to socket or not
+ * --------------------------------
+ */
+void pq_check_client_connection(void)
+{
+ CheckClientConnectionPending = false;
+ if (IsUnderPostmaster &&
+ MyProcPort != NULL && !PqCommReadingMsg && !PqCommBusy)
+ {
+ char nextbyte;
+ int r;
+
+#ifdef WIN32
+ pgwin32_noblock = 1;
+#endif
+ r = recv(MyProcPort->sock, &nextbyte, 1, MSG_PEEK);
+#ifdef WIN32
+ pgwin32_noblock = 0;
+#endif
+
+ if (r == 0 || (r == -1 &&
+ errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR))
+ {
+ ClientConnectionLost = true;
+ InterruptPending = true;
+ }
+ }
+}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8a0332dde9..c9be532362 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3061,6 +3061,8 @@ ProcessInterrupts(void)
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating connection due to administrator command")));
}
+ if (CheckClientConnectionPending)
+ pq_check_client_connection();
if (ClientConnectionLost)
{
QueryCancelPending = false; /* lost connection trumps QueryCancel */
@@ -4288,6 +4290,9 @@ PostgresMain(int argc, char *argv[],
*/
CHECK_FOR_INTERRUPTS();
DoingCommandRead = false;
+ if (client_connection_check_interval)
+ enable_timeout_after(SKIP_CLIENT_CHECK_TIMEOUT,
+ client_connection_check_interval);
/*
* (6) check for any other interesting events that happened while we
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index a5976ad5b1..b42a516bc4 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 e5965bc517..002b6c9fd1 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -34,6 +34,7 @@
#include "catalog/pg_db_role_setting.h"
#include "catalog/pg_tablespace.h"
#include "libpq/auth.h"
+#include "libpq/libpq.h"
#include "libpq/libpq-be.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -73,6 +74,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);
@@ -621,6 +623,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IdleInTransactionSessionTimeoutHandler);
RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
+ RegisterTimeout(SKIP_CLIENT_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
}
/*
@@ -1243,6 +1246,16 @@ IdleSessionTimeoutHandler(void)
SetLatch(MyLatch);
}
+static void
+ClientCheckTimeoutHandler(void)
+{
+ CheckClientConnectionPending = true;
+ InterruptPending = true;
+ if (client_connection_check_interval > 0)
+ enable_timeout_after(SKIP_CLIENT_CHECK_TIMEOUT,
+ client_connection_check_interval);
+}
+
/*
* 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 3fd1a5fbe2..53db7b042e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3446,6 +3446,16 @@ static struct config_int ConfigureNamesInt[] =
NULL, NULL, NULL
},
+ {
+ {"client_connection_check_interval", PGC_USERSET, CLIENT_CONN_OTHER,
+ gettext_noop("Sets the time interval for checking connection with the client."),
+ gettext_noop("A value of -1 disables this feature. Zero selects a suitable default value."),
+ GUC_UNIT_MS
+ },
+ &client_connection_check_interval,
+ 0, 0, INT_MAX,
+ NULL, NULL, NULL
+ },
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ee06528bb0..6609c1426f 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -718,6 +718,9 @@
#dynamic_library_path = '$libdir'
+#client_connection_check_interval = 1000 # set time interval between
+ # connection checks, in ms
+ # 0 is disabled
#------------------------------------------------------------------------------
# LOCK MANAGEMENT
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index b20deeb555..55e605a82e 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 void pq_check_client_connection(void);
/*
* prototypes for functions in be-secure.c
@@ -109,6 +110,7 @@ extern ssize_t secure_open_gssapi(Port *port);
extern char *SSLCipherSuites;
extern char *SSLECDHCurve;
extern bool SSLPreferServerCiphers;
+extern int client_connection_check_interval;
extern int ssl_min_protocol_version;
extern int ssl_max_protocol_version;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1bdc97e308..39fe759e9e 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -84,7 +84,8 @@ extern PGDLLIMPORT volatile sig_atomic_t ProcDiePending;
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 ConfigReloadPending;
+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/utils/timeout.h b/src/include/utils/timeout.h
index ecb2a366a5..89d94ebe18 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,
+ SKIP_CLIENT_CHECK_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,
/* Maximum number of timeout reasons */
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5391f461a2..ac0196f711 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = \
brin \
commit_ts \
+ connection \
delay_execution \
dummy_index_am \
dummy_seclabel \
diff --git a/src/test/modules/connection/Makefile b/src/test/modules/connection/Makefile
new file mode 100644
index 0000000000..5c44d7ad40
--- /dev/null
+++ b/src/test/modules/connection/Makefile
@@ -0,0 +1,16 @@
+# src/test/modules/connection/Makefile
+
+subdir = src/test/modules/connection
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+
+export with_openssl
+
+check:
+ $(prove_check)
+
+installcheck:
+ $(prove_installcheck)
+
+clean distclean maintainer-clean:
+ rm -rf tmp_check
diff --git a/src/test/modules/connection/t/001_close_connection.pl b/src/test/modules/connection/t/001_close_connection.pl
new file mode 100644
index 0000000000..7f727833ec
--- /dev/null
+++ b/src/test/modules/connection/t/001_close_connection.pl
@@ -0,0 +1,107 @@
+# Check if backend stopped after client disconnection
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More;
+use File::Copy;
+
+if ($ENV{with_openssl} eq 'yes')
+{
+ plan tests => 3;
+}
+else
+{
+ plan tests => 2;
+}
+
+my $long_query = q{
+DO
+$$
+DECLARE row_data RECORD;
+BEGIN
+EXECUTE 'CREATE TABLE IF NOT EXISTS keep_alive_test AS SELECT generate_series(0,100000) AS tt';
+FOR row_data IN
+ SELECT tt
+ FROM keep_alive_test
+LOOP
+ EXECUTE 'SELECT count(*) FROM keep_alive_test';
+END LOOP;
+END$$;
+};
+my $set_guc_on = q{
+ SET client_connection_check_interval = 1000;
+};
+my $set_guc_off = q{
+ SET client_connection_check_interval = 0;
+};
+my ($pid, $timed_out);
+
+my $node = get_new_node('node');
+$node->init;
+$node->start;
+
+#########################################################
+# TEST 1: GUC client_connection_check_interval: enabled #
+#########################################################
+
+# Set GUC options, get backend pid and run a long time query
+$node->psql('postgres', "$set_guc_on SELECT pg_backend_pid(); $long_query",
+ stdout => \$pid, timeout => 2, timed_out => \$timed_out);
+
+# Give time to the backend to detect client disconnected
+sleep 3;
+# Check if backend is still alive
+my $is_alive = $node->safe_psql('postgres', "SELECT count(*) FROM pg_stat_activity where pid = $pid;");
+is($is_alive, '0', 'Test: client_connection_check_interval enable');
+$node->stop;
+
+##########################################################
+# TEST 2: GUC client_connection_check_interval: disabled #
+##########################################################
+
+$node->start;
+$node->psql('postgres', "$set_guc_off SELECT pg_backend_pid(); $long_query",
+ stdout => \$pid, timeout => 2, timed_out => \$timed_out);
+# Give time to the client to disconnect
+sleep 3;
+# Check if backend is still alive
+$is_alive = $node->safe_psql('postgres', "SELECT count(*) FROM pg_stat_activity where pid = $pid;");
+is($is_alive, '1', 'Test: client_connection_check_interval disable');
+$node->stop;
+
+##########################################################
+# TEST 3: Using client_connection_check_interval when #
+# client connected using SSL #
+##########################################################
+
+if ($ENV{with_openssl} eq 'yes')
+{
+ # The client's private key must not be world-readable, so take a copy
+ # of the key stored in the code tree and update its permissions.
+ copy("../../ssl/ssl/client.key", "../../ssl/ssl/client_tmp.key");
+ chmod 0600, "../../ssl/ssl/client_tmp.key";
+ copy("../../ssl/ssl/client-revoked.key", "../../ssl/ssl/client-revoked_tmp.key");
+ chmod 0600, "../../ssl/ssl/client-revoked_tmp.key";
+ $ENV{PGHOST} = $node->host;
+ $ENV{PGPORT} = $node->port;
+
+ open my $sslconf, '>', $node->data_dir . "/sslconfig.conf";
+ print $sslconf "ssl=on\n";
+ print $sslconf "ssl_cert_file='server-cn-only.crt'\n";
+ print $sslconf "ssl_key_file='server-password.key'\n";
+ print $sslconf "ssl_passphrase_command='echo secret1'\n";
+ close $sslconf;
+
+ $node->start;
+ $node->psql('postgres', "$set_guc_on SELECT pg_backend_pid(); $long_query",
+ stdout => \$pid, timeout => 2, timed_out => \$timed_out,
+ sslmode => 'require');
+
+ # Give time to the backend to detect client disconnected
+ sleep 3;
+ # Check if backend is still alive
+ my $is_alive = $node->safe_psql('postgres', "SELECT count(*) FROM pg_stat_activity where pid = $pid;");
+ is($is_alive, '0', 'Test: client_connection_check_interval enabled, SSL');
+ $node->stop;
+}
--
2.30.1
From 54e412efbf61fb1fef1dc9444b1bbfc13f7cabe0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Sat, 6 Mar 2021 16:09:39 +1300
Subject: [PATCH v4 2/2] some fixups
- timer handler must set the latch
- rearm the timer from the CFI(), not the signal handler
- only rearm if we're actually still running a query (avoids wakeups
when idle)
- just use pg_sleep() in the test instead of working harder
- SKIP_CLIENT_CHECK_TIMEOUT -> CLIENT_CONNECTION_CHECK_TIMEOUT
- a tiny doc change
---
doc/src/sgml/config.sgml | 2 +-
src/backend/libpq/pqcomm.c | 5 +++++
src/backend/tcop/postgres.c | 8 +++++---
src/backend/utils/init/postinit.c | 6 ++----
src/include/utils/timeout.h | 2 +-
.../modules/connection/t/001_close_connection.pl | 13 +------------
6 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index cadf6a9e7d..a8db1cc806 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -9244,7 +9244,7 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
If the client aborts the connection, the query is terminated.
</para>
<para>
- Default value is <literal>zero</literal> - it disables connection
+ Default value is <literal>zero</literal>. Zero disables connection
checks, so the backend will detect client disconnection only when trying
to send a response to the query.
</para>
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 9f33138105..bf26fda1c0 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -79,6 +79,7 @@
#include "storage/ipc.h"
#include "utils/guc.h"
#include "utils/memutils.h"
+#include "utils/timeout.h"
/*
* Cope with the various platform-specific ways to spell TCP keepalive socket
@@ -1950,5 +1951,9 @@ void pq_check_client_connection(void)
ClientConnectionLost = true;
InterruptPending = true;
}
+
+ if (client_connection_check_interval > 0)
+ enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
+ client_connection_check_interval);
}
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index c9be532362..85a2dcdd5b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2583,6 +2583,11 @@ 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)
+ enable_timeout_after(CLIENT_CONNECTION_CHECK_TIMEOUT,
+ client_connection_check_interval);
}
static void
@@ -4290,9 +4295,6 @@ PostgresMain(int argc, char *argv[],
*/
CHECK_FOR_INTERRUPTS();
DoingCommandRead = false;
- if (client_connection_check_interval)
- enable_timeout_after(SKIP_CLIENT_CHECK_TIMEOUT,
- client_connection_check_interval);
/*
* (6) check for any other interesting events that happened while we
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 002b6c9fd1..2360c38bfd 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -623,7 +623,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
RegisterTimeout(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IdleInTransactionSessionTimeoutHandler);
RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler);
- RegisterTimeout(SKIP_CLIENT_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
+ RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler);
}
/*
@@ -1251,9 +1251,7 @@ ClientCheckTimeoutHandler(void)
{
CheckClientConnectionPending = true;
InterruptPending = true;
- if (client_connection_check_interval > 0)
- enable_timeout_after(SKIP_CLIENT_CHECK_TIMEOUT,
- client_connection_check_interval);
+ SetLatch(MyLatch);
}
/*
diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h
index 89d94ebe18..93e6a691b3 100644
--- a/src/include/utils/timeout.h
+++ b/src/include/utils/timeout.h
@@ -32,7 +32,7 @@ typedef enum TimeoutId
STANDBY_LOCK_TIMEOUT,
IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
IDLE_SESSION_TIMEOUT,
- SKIP_CLIENT_CHECK_TIMEOUT,
+ CLIENT_CONNECTION_CHECK_TIMEOUT,
/* First user-definable timeout reason */
USER_TIMEOUT,
/* Maximum number of timeout reasons */
diff --git a/src/test/modules/connection/t/001_close_connection.pl b/src/test/modules/connection/t/001_close_connection.pl
index 7f727833ec..f688fdb666 100644
--- a/src/test/modules/connection/t/001_close_connection.pl
+++ b/src/test/modules/connection/t/001_close_connection.pl
@@ -16,18 +16,7 @@ else
}
my $long_query = q{
-DO
-$$
-DECLARE row_data RECORD;
-BEGIN
-EXECUTE 'CREATE TABLE IF NOT EXISTS keep_alive_test AS SELECT generate_series(0,100000) AS tt';
-FOR row_data IN
- SELECT tt
- FROM keep_alive_test
-LOOP
- EXECUTE 'SELECT count(*) FROM keep_alive_test';
-END LOOP;
-END$$;
+SELECT pg_sleep(60);
};
my $set_guc_on = q{
SET client_connection_check_interval = 1000;
--
2.30.1
