From afd1d9ec2fceda6fdac8a4c5e9885a6db115df97 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Tue, 28 Dec 2021 12:46:35 +0100 Subject: [PATCH 1/2] 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 | 159 +++++++++++++++++++++++++++--- src/interfaces/libpq/libpq-int.h | 7 ++ 2 files changed, 154 insertions(+), 12 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 9b6a6939f0..ed29c64484 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; } @@ -4426,8 +4473,7 @@ PQfreeCancel(PGcancel *cancel) * between the two versions of the cancel function possible. */ static int -internal_cancel(SockAddr *raddr, int be_pid, int be_key, - char *errbuf, int errbufsize) +internal_cancel(PGcancel *cancel, char *errbuf, int errbufsize) { int save_errno = SOCK_ERRNO; pgsocket tmpsock = PGINVALID_SOCKET; @@ -4443,14 +4489,102 @@ internal_cancel(SockAddr *raddr, int be_pid, int be_key, * We need to open a temporary connection to the postmaster. Do this with * only kernel calls. */ - if ((tmpsock = socket(raddr->addr.ss_family, SOCK_STREAM, 0)) == PGINVALID_SOCKET) + if ((tmpsock = socket(cancel->raddr.addr.ss_family, SOCK_STREAM, 0)) == PGINVALID_SOCKET) { 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 (cancel->pgtcp_user_timeout >= 0) + { + if (setsockopt(tmpsock, IPPROTO_TCP, TCP_USER_TIMEOUT, + (char *) &cancel->pgtcp_user_timeout, + sizeof(cancel->pgtcp_user_timeout)) < 0) + { + 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 (cancel->keepalives_idle >= 0) + { + if (setsockopt(tmpsock, IPPROTO_TCP, PG_TCP_KEEPALIVE_IDLE, + (char *) &cancel->keepalives_idle, + sizeof(cancel->keepalives_idle)) < 0) + { + strlcpy(errbuf, "PQcancel() -- setsockopt(PG_TPC_KEEPALIVE_IDLE) failed: ", errbufsize); + goto cancel_errReturn; + } + } +#endif + +#ifdef TCP_KEEPINTVL + if (cancel->keepalives_interval >= 0) + { + if (setsockopt(tmpsock, IPPROTO_TCP, TCP_KEEPINTVL, + (char *) &cancel->keepalives_interval, + sizeof(cancel->keepalives_interval)) < 0) + { + strlcpy(errbuf, "PQcancel() -- setsockopt(TPC_KEEPINTVL) failed: ", errbufsize); + goto cancel_errReturn; + } + } +#endif + +#ifdef TCP_KEEPCNT + if (cancel->keepalives_count >= 0) + { + if (setsockopt(tmpsock, IPPROTO_TCP, TCP_KEEPCNT, + (char *) &cancel->keepalives_count, + sizeof(cancel->keepalives_count)) < 0) + { + strlcpy(errbuf, "PQcancel() -- setsockopt(TPC_KEEPCNT) failed: ", errbufsize); + goto cancel_errReturn; + } + } +#endif + } +#endif + + retry3: - if (connect(tmpsock, (struct sockaddr *) &raddr->addr, - raddr->salen) < 0) + if (connect(tmpsock, (struct sockaddr *) &cancel->raddr.addr, + cancel->raddr.salen) < 0) { if (SOCK_ERRNO == EINTR) /* Interrupted system call - we'll just try again */ @@ -4467,8 +4601,8 @@ retry3: crp.packetlen = pg_hton32((uint32) sizeof(crp)); crp.cp.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE); - crp.cp.backendPID = pg_hton32(be_pid); - crp.cp.cancelAuthCode = pg_hton32(be_key); + crp.cp.backendPID = pg_hton32(cancel->be_pid); + crp.cp.cancelAuthCode = pg_hton32(cancel->be_key); retry4: if (send(tmpsock, (char *) &crp, sizeof(crp), 0) != (int) sizeof(crp)) @@ -4538,8 +4672,7 @@ PQcancel(PGcancel *cancel, char *errbuf, int errbufsize) return false; } - return internal_cancel(&cancel->raddr, cancel->be_pid, cancel->be_key, - errbuf, errbufsize); + return internal_cancel(cancel, errbuf, errbufsize); } /* @@ -4558,6 +4691,7 @@ int PQrequestCancel(PGconn *conn) { int r; + PGcancel *cancel; /* Check we have an open connection */ if (!conn) @@ -4573,8 +4707,9 @@ PQrequestCancel(PGconn *conn) return false; } - r = internal_cancel(&conn->raddr, conn->be_pid, conn->be_key, - conn->errorMessage.data, conn->errorMessage.maxlen); + cancel = PQgetCancel(conn); + r = PQcancel(cancel, conn->errorMessage.data, conn->errorMessage.maxlen); + PQfreeCancel(cancel); if (!r) conn->errorMessage.len = strlen(conn->errorMessage.data); 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