From 494c949b3a9fe9704bb99689a3dc375fa5156f16 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Fri, 30 Apr 2021 10:48:32 +1200 Subject: [PATCH v6 2/2] Use WL_SOCKET_CLOSED for client_connection_check_interval. Previously we used poll() directly to check for a POLLRDHUP event. Instead, use the WaitEventSet API to poll the socket for WL_SOCKET_CLOSED, which knows how to detect this condition on many more operating systems. Reviewed-by: Zhihong Yu Reviewed-by: Maksim Milyutin Reviewed-by: Andres Freund Discussion: https://postgr.es/m/77def86b27e41f0efcba411460e929ae%40postgrespro.ru --- doc/src/sgml/config.sgml | 6 ++-- src/backend/libpq/pqcomm.c | 54 +++++++++++++++++++++--------------- src/backend/utils/misc/guc.c | 7 ++--- src/include/libpq/libpq.h | 1 + 4 files changed, 37 insertions(+), 31 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..4a6a869460 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1012,9 +1012,9 @@ include_dir 'conf.d' the kernel reports that the connection is closed. - This option is currently available only on systems that support the - non-standard POLLRDHUP extension to the - poll system call, including Linux. + This option relies on kernel events exposed by Linux, macOS, illumos + and the BSD family of operating systems, and is not currently available + on other systems. If the value is specified without units, it is taken as milliseconds. diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 4c37df09cf..3b701998b7 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -204,7 +204,7 @@ pq_init(void) (errmsg("could not set socket to nonblocking mode: %m"))); #endif - FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, 3); + FeBeWaitSet = CreateWaitEventSet(TopMemoryContext, FeBeWaitSetNEvents); socket_pos = AddWaitEventToSet(FeBeWaitSet, WL_SOCKET_WRITEABLE, MyProcPort->sock, NULL, NULL); latch_pos = AddWaitEventToSet(FeBeWaitSet, WL_LATCH_SET, PGINVALID_SOCKET, @@ -1959,33 +1959,41 @@ pq_settcpusertimeout(int timeout, Port *port) 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; + WaitEvent events[FeBeWaitSetNEvents]; + bool result = true; int rc; - pollfd.fd = MyProcPort->sock; - pollfd.events = POLLOUT | POLLIN | POLLRDHUP; - pollfd.revents = 0; + /* + * It's OK to modify the socket event filter without restoring, because + * all FeBeWaitSet socket wait sites do the same. + */ + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetSocketPos, WL_SOCKET_CLOSED, NULL); - rc = poll(&pollfd, 1, 0); + /* + * Temporarily silence the latch, because its higher priority event might + * hide the socket event we want to poll for. + */ + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, NULL); - if (rc < 0) + PG_TRY(); { - ereport(COMMERROR, - (errcode_for_socket_access(), - errmsg("could not poll socket: %m"))); - return false; + rc = WaitEventSetWait(FeBeWaitSet, 0, events, lengthof(events), 0); + for (int i = 0; i < rc; ++i) + { + if (events[i].events & WL_SOCKET_CLOSED) + result = false; + } } - else if (rc == 1 && (pollfd.revents & (POLLHUP | POLLRDHUP))) - return false; -#endif + PG_FINALLY(); + { + /* + * If WaitEventSetWait() reports an error, something must be pretty + * seriously wrong, but we should restore the latch on principle. + */ + ModifyWaitEvent(FeBeWaitSet, FeBeWaitSetLatchPos, WL_LATCH_SET, + MyLatch); + } + PG_END_TRY(); - return true; + return result; } diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index f9504d3aec..57292b2364 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -12129,14 +12129,11 @@ check_huge_page_size(int *newval, void **extra, GucSource source) 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) + if (!WaitEventSetCanReportClosed() && *newval != 0) { - GUC_check_errdetail("client_connection_check_interval must be set to 0 on platforms that lack POLLRDHUP."); + GUC_check_errdetail("client_connection_check_interval must be set to 0 on this platform"); return false; } -#endif return true; } diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index 6b67a2a318..9f168a284f 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -62,6 +62,7 @@ extern WaitEventSet *FeBeWaitSet; #define FeBeWaitSetSocketPos 0 #define FeBeWaitSetLatchPos 1 +#define FeBeWaitSetNEvents 3 extern int StreamServerPort(int family, const char *hostName, unsigned short portNumber, const char *unixSocketDir, -- 2.33.1