Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lQve9-0002rq-8S for pgsql-hackers@arkaria.postgresql.org; Mon, 29 Mar 2021 17:25:49 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lQve8-0005Ck-41 for pgsql-hackers@arkaria.postgresql.org; Mon, 29 Mar 2021 17:25:48 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lQve7-0005Cd-R5 for pgsql-hackers@lists.postgresql.org; Mon, 29 Mar 2021 17:25:47 +0000 Received: from mail-lf1-x134.google.com ([2a00:1450:4864:20::134]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1lQve4-0005ZM-V1 for pgsql-hackers@postgresql.org; Mon, 29 Mar 2021 17:25:47 +0000 Received: by mail-lf1-x134.google.com with SMTP id g8so19602664lfv.12 for ; Mon, 29 Mar 2021 10:25:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language; bh=DsG5skzGpkzZZRt27c1mzNlP+kzkcOqnzSgj4bwawL0=; b=f+U3gXPo6k7jus0NRG3aGZvoRva6NyFQHoaZdDuYAs4cA8DTbWB98GwrHWxX7U9GRF RIU0avg9G4vq7PVW5wCsNS0xdvGnDNDzLsqyWb8md8tkRasY/JT+dYPH06qUrg890HgR kVsv3I04aYqXTX/tQKmChk9i/O/EonWVxnKFbDXO5/tpRoF7hjIDxThKarIOnBZhbqSM +u9bbaqUy61TmE0o+7WO8oHYUD3s6sYrXc8rHWegUS6n9FTAyuPtMkfxYIcbbVqP6AVQ Mocw5Jqom5dtlwwgUm1tNLdNHnfuxSJJBTQPWp/hZsN3VK4pq0f9+HUM4e2E6Y1PnE+F NEqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=DsG5skzGpkzZZRt27c1mzNlP+kzkcOqnzSgj4bwawL0=; b=eoXGX7o7GZbNGby8UQIcb+vrKfNH7wXug/1Ou4DTpb3ya8PcKH/scKiBA9Y22Hffr1 l77NFLoDG2/CfgURkPMBz5AJvieMt8n6YSObE51HeCXlhwGnICk8vqKZFLksRdv7BcS+ 3ND1kHVkoK4j1RRQNn3QTXghiRLSb7uJi1FBZkrYCfdUKJGmwiU15DDlY2t7j6q9oCqE eB9iNipYMK5dlTrgMuHC94jldsZpWqCDjm+FfuQzjWvUKJ6AMCr01fYaJ3ks+7TcP5Q1 P7jIVpQXVObPBFwDLeQjC6Z/pU5eBKh9izLWbdBFR8ZmJ7r/QmLChzMwXyB8K9mxEi8y pFSw== X-Gm-Message-State: AOAM533bN2W7FwQg8RV7JLhEbU73cWjImS5Kr1iWrNx5pooARUt3R3J0 hCeqytRVeYbtkQZURl2n0DI= X-Google-Smtp-Source: ABdhPJxagxIpR03urWL/n8mCvwMFaG7XrKfpwWhpwTcKgalhZhtYyp5yB39VJNAJ7jcJMJq/Kpdx3Q== X-Received: by 2002:ac2:47ea:: with SMTP id b10mr16397023lfp.499.1617038744194; Mon, 29 Mar 2021 10:25:44 -0700 (PDT) Received: from [192.168.0.12] (59efa4ef.dynamic.mv.ru. [89.239.164.239]) by smtp.gmail.com with ESMTPSA id u15sm1979972lfl.110.2021.03.29.10.25.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 29 Mar 2021 10:25:43 -0700 (PDT) Subject: Re: Add client connection check during the execution of the query To: Thomas Munro , pgsql-hackers Cc: Tatsuo Ishii , Tom Lane , Andres Freund , Zhihong Yu References: <37D9769E-2991-46A9-A65B-498E3CF55D8C@postgrespro.ru> <20190718.121906.432767475415875714.t-ishii@sraoss.co.jp> <79f5f12b-184f-dcdb-b22f-4f95b6a89f2d@postgrespro.ru> From: Maksim Milyutin Message-ID: <6bd6945e-f02e-8ee8-b8c4-e8d74e283efd@gmail.com> Date: Mon, 29 Mar 2021 20:25:42 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------B99C93D2A8EE73396A586406" Content-Language: en-US List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------B99C93D2A8EE73396A586406 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Hi Thomas! Thanks for working on this patch. I have attached a new version with some typo corrections of doc entry, removing of redundant `include` entries and trailing whitespaces. Also I added in doc the case when single query transaction with disconnected client might be eventually commited upon completion in autocommit mode if it doesn't return any rows (doesn't communicate with user) before sending final commit message. This behavior might be unexpected for clients and hence IMO it's worth noticing. > diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c > index 4c7b1e7bfd..8cf95d09a4 100644 > --- a/src/backend/libpq/pqcomm.c > +++ b/src/backend/libpq/pqcomm.c > > @@ -919,13 +923,13 @@ socket_set_nonblocking(bool nonblocking) >  } > >  /* -------------------------------- > - *        pq_recvbuf - load some bytes into the input buffer > + *        pq_recvbuf_ext - load some bytes into the input buffer >   * >   *        returns 0 if OK, EOF if trouble >   * -------------------------------- >   */ >  static int > -pq_recvbuf(void) > +pq_recvbuf_ext(bool nonblocking) >  { >      if (PqRecvPointer > 0) >      { > @@ -941,8 +945,7 @@ pq_recvbuf(void) >              PqRecvLength = PqRecvPointer = 0; >      } > > -    /* Ensure that we're in blocking mode */ > -    socket_set_nonblocking(false); > +    socket_set_nonblocking(nonblocking); > >      /* Can fill buffer from PqRecvLength and upwards */ >      for (;;) > @@ -954,6 +957,9 @@ pq_recvbuf(void) > >          if (r < 0) >          { > +            if (nonblocking && (errno == EAGAIN || errno == EWOULDBLOCK)) > +                return 0; > + >              if (errno == EINTR) >                  continue;        /* Ok if interrupted */ > > @@ -981,6 +987,13 @@ pq_recvbuf(void) >      } >  } > > +static int > +pq_recvbuf(void) > +{ > +    return pq_recvbuf_ext(false); > +} > + > + AFAICS, the above fragment is not related with primary fix directly. AFAICS, there are the following open items that don't allow to treat the current patch completed: * Absence of tap tests emulating some scenarios of client disconnection. IIRC, you wanted to rewrite the test case posted by Sergey. * Concerns about portability of `pq_check_connection()`A implementation. BTW, on windows postgres with this patch have not been built [1]. * Absence of benchmark results to show lack of noticeable performance regression after applying non-zero timeout for checking client liveness. 1. https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131820 -- Regards, Maksim Milyutin --------------B99C93D2A8EE73396A586406 Content-Type: text/x-patch; charset=UTF-8; name="v9-0001-Detect-POLLHUP-while-running-queries.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="v9-0001-Detect-POLLHUP-while-running-queries.patch" From 3ec788ac5e7c47fe135a3618849db179942f4b27 Mon Sep 17 00:00:00 2001 From: Maksim Milyutin Date: Fri, 26 Mar 2021 10:18:30 +0300 Subject: [PATCH v9] Detect POLLHUP 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 Author: Thomas Munro Reviewed-by: Thomas Munro Reviewed-by: Tatsuo Ishii Reviewed-by: Konstantin Knizhnik Reviewed-by: Zhihong Yu Reviewed-by: Andres Freund Reviewed-by: Maksim Milyutin Reviewed-by: Tom Lane (much earlier version) Discussion: https://postgr.es/m/77def86b27e41f0efcba411460e929ae%40postgrespro.ru --- doc/src/sgml/config.sgml | 36 +++++++++++ src/backend/libpq/pqcomm.c | 63 +++++++++++++++++-- src/backend/tcop/postgres.c | 27 ++++++++ src/backend/utils/init/globals.c | 1 + src/backend/utils/init/postinit.c | 10 +++ 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 | 1 + src/include/utils/timeout.h | 1 + 10 files changed, 150 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index ddc6d789d8..abec47ada7 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -998,6 +998,42 @@ include_dir 'conf.d' + + client_connection_check_interval (integer) + + client_connection_check_interval configuration parameter + + + + + Sets the time interval between checks that the client is still + connected, while running queries. The check is performed by polling the + socket to allows long running queries to be aborted immediately. + + + If this value is specified without units, it is taken as milliseconds. + The default value is 0, which disables connection + checks. Without connection checks, the server will detect the loss of + the connection only after next interaction with client through + connection socket: when it starts to wait for a new request, receives a + request or sends a response. As a consequence, some modificatory single + query transaction in autocommit mode that doesn't return any rows + becomes commited somewhen later silently and unexpectedly for previously + disconnected client. + + + 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 default TCP keepalive settings of the + operating system, or the + , + and + settings of + PostgreSQL. + + + + diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 4c7b1e7bfd..8cf95d09a4 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -59,6 +59,9 @@ #include #include #include +#ifdef HAVE_POLL_H +#include +#endif #include #include #include @@ -104,6 +107,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; @@ -919,13 +923,13 @@ socket_set_nonblocking(bool nonblocking) } /* -------------------------------- - * pq_recvbuf - load some bytes into the input buffer + * pq_recvbuf_ext - load some bytes into the input buffer * * returns 0 if OK, EOF if trouble * -------------------------------- */ static int -pq_recvbuf(void) +pq_recvbuf_ext(bool nonblocking) { if (PqRecvPointer > 0) { @@ -941,8 +945,7 @@ pq_recvbuf(void) PqRecvLength = PqRecvPointer = 0; } - /* Ensure that we're in blocking mode */ - socket_set_nonblocking(false); + socket_set_nonblocking(nonblocking); /* Can fill buffer from PqRecvLength and upwards */ for (;;) @@ -954,6 +957,9 @@ pq_recvbuf(void) if (r < 0) { + if (nonblocking && (errno == EAGAIN || errno == EWOULDBLOCK)) + return 0; + if (errno == EINTR) continue; /* Ok if interrupted */ @@ -981,6 +987,13 @@ pq_recvbuf(void) } } +static int +pq_recvbuf(void) +{ + return pq_recvbuf_ext(false); +} + + /* -------------------------------- * pq_getbyte - get a single byte from connection, or return EOF * -------------------------------- @@ -1921,3 +1934,45 @@ pq_settcpusertimeout(int timeout, Port *port) return STATUS_OK; } + +/* + * POLLRDHUP is a Linux extension to poll(2). Unlike POLLHUP, it has to be + * requested explicitly. It detects half-shutdown sockets, which are not + * always reported as POLLHUP on Linux, depending on the type of socket. We'll + * look out for both. + */ +#ifdef POLLRDHUP +#define PG_POLLRDHUP POLLRDHUP +#else +#define PG_POLLRDHUP 0 +#endif + +/* + * Check if the client is still connected. + */ +bool +pq_check_connection(void) +{ +#if defined(HAVE_POLL) || defined(WIN32) + struct pollfd pollfd; + int rc; + + pollfd.fd = MyProcPort->sock; + pollfd.events = POLLOUT | POLLIN | PG_POLLRDHUP; + pollfd.revents = 0; +#ifdef WIN32 + rc = WSAPoll(&pollfd, 1, 0); +#else + rc = poll(&pollfd, 1, 0); +#endif + if (rc < 0) + { + elog(COMMERROR, "could not poll socket: %m"); + return false; + } + else if (rc == 1 && (pollfd.revents & (POLLHUP | PG_POLLRDHUP))) + return false; +#endif + + return true; +} diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 2b1b68109f..b04573cd43 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -2671,6 +2671,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 +3157,25 @@ 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 0c5dc4d3e8..9f67e8140e 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3481,6 +3481,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 0 disables this feature."), + 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 b234a6bfe6..9691cb86f1 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -717,6 +717,9 @@ #dynamic_library_path = '$libdir' +#client_connection_check_interval = 0 # set time interval between + # checks for client disconnection while + # running long queries; 0 for never #------------------------------------------------------------------------------ # LOCK MANAGEMENT diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index b20deeb555..fac233c9f4 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 @@ -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 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/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.25.1 --------------B99C93D2A8EE73396A586406--