From f53bb6b006ddd92798b3176a876e0cca6d238a69 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Thu, 6 Jan 2022 15:39:27 +0100 Subject: [PATCH 2/3] Use timeout socket options for cancel connections PQcancel would not adhere to the timeout specified in the tcp_user_timeout connection option. It would also not enable keepalives on the socket. So no keepalives would be used by the on these connections at all, not even the system configured ones. This means a call to PQcancel could take much longer than expected, possibly indefinitely. This can especially be an issue because there's no non-blocking version of PQcancel. This patch correctly sets both tcp_user_timeout and keepalive related socket options on the connection that's used to cancel a query. It only does so for non Windows systems, because the signal safety of the related windows functions is not clear to me. --- src/interfaces/libpq/fe-connect.c | 135 +++++++++++++++++++++++++++++- src/interfaces/libpq/libpq-int.h | 7 ++ 2 files changed, 141 insertions(+), 1 deletion(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 85fce856a5..49d183d13c 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -4385,13 +4385,60 @@ PQgetCancel(PGconn *conn) if (conn->sock == PGINVALID_SOCKET) return NULL; - cancel = malloc(sizeof(PGcancel)); + cancel = calloc(1, sizeof(PGcancel)); if (cancel == NULL) return NULL; memcpy(&cancel->raddr, &conn->raddr, sizeof(SockAddr)); cancel->be_pid = conn->be_pid; cancel->be_key = conn->be_key; + /* Set all the socket options to -1, so we can use that to see if the */ + /* socket options are set or not. */ + cancel->pgtcp_user_timeout = -1; + cancel->keepalives = -1; + cancel->keepalives_idle = -1; + cancel->keepalives_interval = -1; + cancel->keepalives_count = -1; + if (conn->pgtcp_user_timeout != NULL) + { + if (!parse_int_param(conn->pgtcp_user_timeout, &cancel->pgtcp_user_timeout, conn, + "tcp_user_timeout")) + { + return NULL; + } + } + if (conn->keepalives != NULL) + { + if (!parse_int_param(conn->keepalives, &cancel->keepalives, conn, + "keepalives")) + { + return NULL; + } + } + if (conn->keepalives_idle != NULL) + { + if (!parse_int_param(conn->keepalives_idle, &cancel->keepalives_idle, conn, + "keepalives_idle")) + { + return NULL; + } + } + if (conn->keepalives_interval != NULL) + { + if (!parse_int_param(conn->keepalives_interval, &cancel->keepalives_interval, conn, + "keepalives_interval")) + { + return NULL; + } + } + if (conn->keepalives_count != NULL) + { + if (!parse_int_param(conn->keepalives_count, &cancel->keepalives_count, conn, + "keepalives_count")) + { + return NULL; + } + } return cancel; } @@ -4405,6 +4452,28 @@ PQfreeCancel(PGcancel *cancel) } +/* + * Sets an integer based socket options on TCP socket if the provided value is + * not negative. Returns false if setsockopt fails for some reason. + * + * CAUTION: This needs to be signal safe, since it's used by PQcancel. + */ +static bool +set_optional_int_tcp_sockopt(int fd, int optname, int value) { + if (value < 0) + { + return true; + } + if (setsockopt(fd, IPPROTO_TCP, optname, + (char *) &value, + sizeof(value)) < 0) + { + return false; + } + return true; +} + + /* * PQcancel: request query cancel * @@ -4453,6 +4522,70 @@ PQcancel(PGcancel *cancel, char *errbuf, int errbufsize) strlcpy(errbuf, "PQcancel() -- socket() failed: ", errbufsize); goto cancel_errReturn; } + + /* + * Since this connection will only be used to send a tiny bit of data, we + * don't need NODELAY. We also don't set the socket to nonblocking mode, + * because the API definition of PQcancel currently requires the cancel to + * be sent in a blocking way. + * + * We do set various other socket options related to keepalives and other + * TCP timeouts. This is necessary to make sure that the call to this + * function does not block indefinitly, when reasonable keepalive and + * settings have been provided. + * + * NOTE: These socket options are currently not set for Windows. The + * reason is that signal safety in this function is very important, and it + * was not clear to if the functions required to set the socket options on + * Windows were signal-safe. + */ +#ifndef WIN32 + if (!IS_AF_UNIX(cancel->raddr.addr.ss_family)) + { +#ifdef TCP_USER_TIMEOUT + if (!set_optional_int_tcp_sockopt(tmpsock, TCP_USER_TIMEOUT, cancel->pgtcp_user_timeout)) { + strlcpy(errbuf, "PQcancel() -- setsockopt(TCP_USER_TIMEOUT) failed: ", errbufsize); + goto cancel_errReturn; + } +#endif + + if (cancel->keepalives != 0) + { + int on = 1; + + if (setsockopt(tmpsock, + SOL_SOCKET, SO_KEEPALIVE, + (char *) &on, sizeof(on)) < 0) + { + strlcpy(errbuf, "PQcancel() -- setsockopt(SO_KEEPALIVE) failed: ", errbufsize); + goto cancel_errReturn; + } + } + +#ifdef PG_TCP_KEEPALIVE_IDLE + if (!set_optional_int_tcp_sockopt(tmpsock, PG_TCP_KEEPALIVE_IDLE, cancel->keepalives_idle)) { + strlcpy(errbuf, "PQcancel() -- setsockopt(" PG_TCP_KEEPALIVE_IDLE_STR ") failed: ", errbufsize); + goto cancel_errReturn; + } +#endif + +#ifdef TCP_KEEPINTVL + if (!set_optional_int_tcp_sockopt(tmpsock, TCP_KEEPINTVL, cancel->keepalives_interval)) { + strlcpy(errbuf, "PQcancel() -- setsockopt(TCP_KEEPINTVL) failed: ", errbufsize); + goto cancel_errReturn; + } +#endif + +#ifdef TCP_KEEPCNT + if (!set_optional_int_tcp_sockopt(tmpsock, TCP_KEEPCNT, cancel->keepalives_count)) { + strlcpy(errbuf, "PQcancel() -- setsockopt(TCP_KEEPCNT) failed: ", errbufsize); + goto cancel_errReturn; + } +#endif + } +#endif + + retry3: if (connect(tmpsock, (struct sockaddr *) &cancel->raddr.addr, cancel->raddr.salen) < 0) diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 490458adef..6dabb14451 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -583,6 +583,13 @@ struct pg_cancel SockAddr raddr; /* Remote address */ int be_pid; /* PID of backend --- needed for cancels */ int be_key; /* key of backend --- needed for cancels */ + int pgtcp_user_timeout; /* tcp user timeout */ + int keepalives; /* use TCP keepalives? */ + int keepalives_idle; /* time between TCP keepalives */ + int keepalives_interval; /* time between TCP keepalive + * retransmits */ + int keepalives_count; /* maximum number of TCP keepalive + * retransmits */ }; -- 2.17.1