On Sat, Aug 3, 2019 at 4:40 AM Konstantin Knizhnik <k.knizh...@postgrespro.ru> wrote: > On 18.07.2019 6:19, Tatsuo Ishii wrote: > > So the performance is about 5% down with the feature enabled in this > > case. For me, 5% down is not subtle. Probably we should warn this in > > the doc. > > I also see some performance degradation, although it is not so large in > my case (I used pgbench with scale factor 10 and run the same command as > you). > In my case difference is 103k vs. 105k TPS is smaller than 2%.
I didn't test, but hopefully the degradation is fixed by commit 09cf1d52? > If OS detected closed connection, it should return POLLHUP, should not it? > I am not sure if it is more portable or more efficient way - just seems > to be a little bit more natural way (from my point of view) to check if > connection is still alive. That's if you're sleeping inepoll etc. This patch is for CPU-bound backends, running a long query. We need to do something special to find out if the kernel knows that the connection has been closed. 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.
From 5f7c327c5896369b80529467a3f1f64eab690887 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 v3] 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 b5718fc136..db8798eb95 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -9220,6 +9220,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 27a298f110..44fb79c2b5 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -118,6 +118,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; @@ -2022,3 +2023,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 bb5ccb4578..4ce004a2d5 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3128,6 +3128,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 */ @@ -4355,6 +4357,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 d626731723..8700703af6 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3445,6 +3445,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 e4e5c21565..149af0d0b6 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -76,6 +76,7 @@ extern int pq_getbyte(void); extern int pq_peekbyte(void); extern int pq_getbyte_if_available(unsigned char *c); extern int pq_putbytes(const char *s, size_t len); +extern void pq_check_client_connection(void); /* * prototypes for functions in be-secure.c @@ -114,6 +115,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.0
