From 5ca00774194dc179d02867d536b73eb85fffd227 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 23 Aug 2018 16:39:19 -0400 Subject: [PATCH] In libpq, don't look up all the hostnames at once. Historically, we looked up the target hostname in connectDBStart, so that PQconnectPoll did not need to do DNS name resolution. The patches that added multiple-target-host support to libpq preserved this division of labor; but it's really nonsensical now, because it means that if any one of the target hosts fails to resolve in DNS, the connection fails. That negates the no-single-point-of-failure goal of the feature. Additionally, DNS lookups aren't exactly cheap, but the code did them all even if the first connection attempt succeeds. Hence, rearrange so that PQconnectPoll does the lookups, and only looks up a hostname when it's time to try that host. This does mean that PQconnectPoll could block on a DNS lookup --- but if you wanted to avoid that, you should be using hostaddr, as the documentation has always specified. It seems fairly unlikely that any applications would really care whether the lookup occurs inside PQconnectStart or PQconnectPoll. In addition to calling out that fact explicitly, do some other minor wordsmithing in the docs around the multiple-target-host feature. Since this seems like a bug in the multiple-target-host feature, backpatch to v10 where that was introduced. In the back branches, avoid moving any existing fields of struct pg_conn, just in case any third-party code is looking into that struct. Tom Lane, reviewed by Fabien Coelho Discussion: https://postgr.es/m/4913.1533827102@sss.pgh.pa.us --- doc/src/sgml/libpq.sgml | 78 ++++++---- src/interfaces/libpq/fe-connect.c | 249 ++++++++++++++---------------- src/interfaces/libpq/libpq-int.h | 5 +- 3 files changed, 164 insertions(+), 168 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 49de975e7f..5e7931ba90 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -303,9 +303,9 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn); - The hostaddr and host parameters are used appropriately to ensure that - name and reverse name queries are not made. See the documentation of - these parameters in for details. + The hostaddr parameter must be used appropriately + to prevent DNS queries from being made. See the documentation of + this parameter in for details. @@ -318,7 +318,7 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn); - You ensure that the socket is in the appropriate state + You must ensure that the socket is in the appropriate state before calling PQconnectPoll, as described below. @@ -326,24 +326,27 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn); - Note: use of PQconnectStartParams is analogous to - PQconnectStart shown below. + To begin a nonblocking connection request, + call PQconnectStart + or PQconnectStartParams. If the result is null, + then libpq has been unable to allocate a + new PGconn structure. Otherwise, a + valid PGconn pointer is returned (though not + yet representing a valid connection to the database). Next + call PQstatus(conn). If the result + is CONNECTION_BAD, the connection attempt has already + failed, typically because of invalid connection parameters. - To begin a nonblocking connection request, call conn = PQconnectStart("connection_info_string"). - If conn is null, then libpq has been unable to allocate a new PGconn - structure. Otherwise, a valid PGconn pointer is returned (though not yet - representing a valid connection to the database). On return from - PQconnectStart, call status = PQstatus(conn). If status equals - CONNECTION_BAD, PQconnectStart has failed. - - - - If PQconnectStart succeeds, the next stage is to poll - libpq so that it can proceed with the connection sequence. + If PQconnectStart + or PQconnectStartParams succeeds, the next stage + is to poll libpq so that it can proceed with + the connection sequence. Use PQsocket(conn) to obtain the descriptor of the socket underlying the database connection. + (Caution: do not assume that the socket remains the same + across PQconnectPoll calls.) Loop thus: If PQconnectPoll(conn) last returned PGRES_POLLING_READING, wait until the socket is ready to read (as indicated by select(), poll(), or @@ -352,9 +355,8 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn); Conversely, if PQconnectPoll(conn) last returned PGRES_POLLING_WRITING, wait until the socket is ready to write, then call PQconnectPoll(conn) again. - If you have yet to call - PQconnectPoll, i.e., just after the call to - PQconnectStart, behave as if it last returned + On the first iteration, i.e. if you have yet to call + PQconnectPoll, behave as if it last returned PGRES_POLLING_WRITING. Continue this loop until PQconnectPoll(conn) returns PGRES_POLLING_FAILED, indicating the connection procedure @@ -479,10 +481,12 @@ switch(PQstatus(conn)) - Note that if PQconnectStart returns a non-null pointer, you must call - PQfinish when you are finished with it, in order to dispose of - the structure and any associated memory blocks. This must be done even if - the connection attempt fails or is abandoned. + Note that when PQconnectStart + or PQconnectStartParams returns a non-null + pointer, you must call PQfinish when you are + finished with it, in order to dispose of the structure and any + associated memory blocks. This must be done even if the connection + attempt fails or is abandoned. @@ -913,7 +917,8 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname It is possible to specify multiple hosts to connect to, so that they are tried in the given order. In the Keyword/Value format, the host, hostaddr, and port options accept a comma-separated - list of values. The same number of elements must be given in each option, such + list of values. The same number of elements must be given in each + option that is specified, such that e.g. the first hostaddr corresponds to the first host name, the second hostaddr corresponds to the second host name, and so forth. As an exception, if only one port is specified, it @@ -922,9 +927,13 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname In the connection URI format, you can list multiple host:port pairs - separated by commas, in the host component of the URI. In either - format, a single host name can also translate to multiple network addresses. A - common example of this is a host that has both an IPv4 and an IPv6 address. + separated by commas, in the host component of the URI. + + + + In either format, a single host name can translate to multiple network + addresses. A common example of this is a host that has both an IPv4 and + an IPv6 address. @@ -958,9 +967,8 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname Name of host to connect to.host name If a host name begins with a slash, it specifies Unix-domain communication rather than TCP/IP communication; the value is the - name of the directory in which the socket file is stored. If - multiple host names are specified, each will be tried in turn in - the order given. The default behavior when host is + name of the directory in which the socket file is stored. + The default behavior when host is not specified, or is empty, is to connect to a Unix-domain socketUnix domain socket in /tmp (or whatever socket directory was specified @@ -997,8 +1005,12 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname - If host is specified without hostaddr, - a host name lookup occurs. + If host is specified + without hostaddr, a host name lookup occurs. + (When using PQconnectPoll, the lookup occurs + when PQconnectPoll first considers this host + name, and it may cause PQconnectPoll to block + for a significant amount of time.) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 9315a27561..a8048ffad2 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -360,7 +360,7 @@ static PGconn *makeEmptyPGconn(void); static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions); static void freePGconn(PGconn *conn); static void closePGconn(PGconn *conn); -static void release_all_addrinfo(PGconn *conn); +static void release_conn_addrinfo(PGconn *conn); static void sendTerminateConn(PGconn *conn); static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage); static PQconninfoOption *parse_connection_string(const char *conninfo, @@ -1742,10 +1742,6 @@ setKeepalivesWin32(PGconn *conn) static int connectDBStart(PGconn *conn) { - char portstr[MAXPGPATH]; - int ret; - int i; - if (!conn) return 0; @@ -1764,101 +1760,6 @@ connectDBStart(PGconn *conn) */ resetPQExpBuffer(&conn->errorMessage); - /* - * Look up socket addresses for each possible host using - * pg_getaddrinfo_all. - */ - for (i = 0; i < conn->nconnhost; ++i) - { - pg_conn_host *ch = &conn->connhost[i]; - struct addrinfo hint; - int thisport; - - /* Initialize hint structure */ - MemSet(&hint, 0, sizeof(hint)); - hint.ai_socktype = SOCK_STREAM; - hint.ai_family = AF_UNSPEC; - - /* Figure out the port number we're going to use. */ - if (ch->port == NULL || ch->port[0] == '\0') - thisport = DEF_PGPORT; - else - { - thisport = atoi(ch->port); - if (thisport < 1 || thisport > 65535) - { - appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("invalid port number: \"%s\"\n"), - ch->port); - conn->options_valid = false; - goto connect_errReturn; - } - } - snprintf(portstr, sizeof(portstr), "%d", thisport); - - /* Use pg_getaddrinfo_all() to resolve the address */ - ret = 1; - switch (ch->type) - { - case CHT_HOST_NAME: - ret = pg_getaddrinfo_all(ch->host, portstr, &hint, &ch->addrlist); - if (ret || !ch->addrlist) - appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("could not translate host name \"%s\" to address: %s\n"), - ch->host, gai_strerror(ret)); - break; - - case CHT_HOST_ADDRESS: - hint.ai_flags = AI_NUMERICHOST; - ret = pg_getaddrinfo_all(ch->hostaddr, portstr, &hint, &ch->addrlist); - if (ret || !ch->addrlist) - appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("could not parse network address \"%s\": %s\n"), - ch->hostaddr, gai_strerror(ret)); - break; - - case CHT_UNIX_SOCKET: -#ifdef HAVE_UNIX_SOCKETS - hint.ai_family = AF_UNIX; - UNIXSOCK_PATH(portstr, thisport, ch->host); - if (strlen(portstr) >= UNIXSOCK_PATH_BUFLEN) - { - appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("Unix-domain socket path \"%s\" is too long (maximum %d bytes)\n"), - portstr, - (int) (UNIXSOCK_PATH_BUFLEN - 1)); - conn->options_valid = false; - goto connect_errReturn; - } - - /* - * NULL hostname tells pg_getaddrinfo_all to parse the service - * name as a Unix-domain socket path. - */ - ret = pg_getaddrinfo_all(NULL, portstr, &hint, &ch->addrlist); - if (ret || !ch->addrlist) - appendPQExpBuffer(&conn->errorMessage, - libpq_gettext("could not translate Unix-domain socket path \"%s\" to address: %s\n"), - portstr, gai_strerror(ret)); - break; -#else - Assert(false); - conn->options_valid = false; - goto connect_errReturn; -#endif - } - if (ret || !ch->addrlist) - { - if (ch->addrlist) - { - pg_freeaddrinfo_all(hint.ai_family, ch->addrlist); - ch->addrlist = NULL; - } - conn->options_valid = false; - goto connect_errReturn; - } - } - /* * Set up to try to connect to the first host. (Setting whichhost = -1 is * a bit of a cheat, but PQconnectPoll will advance it to 0 before @@ -2157,6 +2058,12 @@ keep_going: /* We will come back to here until there is /* Time to advance to next connhost[] entry? */ if (conn->try_next_host) { + pg_conn_host *ch; + struct addrinfo hint; + int thisport; + int ret; + char portstr[MAXPGPATH]; + if (conn->whichhost + 1 >= conn->nconnhost) { /* @@ -2166,10 +2073,100 @@ keep_going: /* We will come back to here until there is goto error_return; } conn->whichhost++; - conn->addr_cur = conn->connhost[conn->whichhost].addrlist; - /* If no addresses for this host, just try the next one */ - if (conn->addr_cur == NULL) - goto keep_going; + + /* Drop any address info for previous host */ + release_conn_addrinfo(conn); + + /* + * Look up info for the new host. On failure, log the problem in + * conn->errorMessage, then loop around to try the next host. (Note + * we don't clear try_next_host until we've succeeded.) + */ + ch = &conn->connhost[conn->whichhost]; + + /* Initialize hint structure */ + MemSet(&hint, 0, sizeof(hint)); + hint.ai_socktype = SOCK_STREAM; + conn->addrlist_family = hint.ai_family = AF_UNSPEC; + + /* Figure out the port number we're going to use. */ + if (ch->port == NULL || ch->port[0] == '\0') + thisport = DEF_PGPORT; + else + { + thisport = atoi(ch->port); + if (thisport < 1 || thisport > 65535) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("invalid port number: \"%s\"\n"), + ch->port); + goto keep_going; + } + } + snprintf(portstr, sizeof(portstr), "%d", thisport); + + /* Use pg_getaddrinfo_all() to resolve the address */ + switch (ch->type) + { + case CHT_HOST_NAME: + ret = pg_getaddrinfo_all(ch->host, portstr, &hint, + &conn->addrlist); + if (ret || !conn->addrlist) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("could not translate host name \"%s\" to address: %s\n"), + ch->host, gai_strerror(ret)); + goto keep_going; + } + break; + + case CHT_HOST_ADDRESS: + hint.ai_flags = AI_NUMERICHOST; + ret = pg_getaddrinfo_all(ch->hostaddr, portstr, &hint, + &conn->addrlist); + if (ret || !conn->addrlist) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("could not parse network address \"%s\": %s\n"), + ch->hostaddr, gai_strerror(ret)); + goto keep_going; + } + break; + + case CHT_UNIX_SOCKET: +#ifdef HAVE_UNIX_SOCKETS + conn->addrlist_family = hint.ai_family = AF_UNIX; + UNIXSOCK_PATH(portstr, thisport, ch->host); + if (strlen(portstr) >= UNIXSOCK_PATH_BUFLEN) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("Unix-domain socket path \"%s\" is too long (maximum %d bytes)\n"), + portstr, + (int) (UNIXSOCK_PATH_BUFLEN - 1)); + goto keep_going; + } + + /* + * NULL hostname tells pg_getaddrinfo_all to parse the service + * name as a Unix-domain socket path. + */ + ret = pg_getaddrinfo_all(NULL, portstr, &hint, + &conn->addrlist); + if (ret || !conn->addrlist) + { + appendPQExpBuffer(&conn->errorMessage, + libpq_gettext("could not translate Unix-domain socket path \"%s\" to address: %s\n"), + portstr, gai_strerror(ret)); + goto keep_going; + } +#else + Assert(false); +#endif + break; + } + + /* OK, scan this addrlist for a working server address */ + conn->addr_cur = conn->addrlist; reset_connection_state_machine = true; conn->try_next_host = false; } @@ -3134,8 +3131,8 @@ keep_going: /* We will come back to here until there is return PGRES_POLLING_READING; } - /* We can release the address lists now. */ - release_all_addrinfo(conn); + /* We can release the address list now. */ + release_conn_addrinfo(conn); /* We are open for business! */ conn->status = CONNECTION_OK; @@ -3197,8 +3194,8 @@ keep_going: /* We will come back to here until there is return PGRES_POLLING_READING; } - /* We can release the address lists now. */ - release_all_addrinfo(conn); + /* We can release the address list now. */ + release_conn_addrinfo(conn); /* We are open for business! */ conn->status = CONNECTION_OK; @@ -3227,6 +3224,9 @@ keep_going: /* We will come back to here until there is goto keep_going; } + /* We can release the address list now. */ + release_conn_addrinfo(conn); + /* We are open for business! */ conn->status = CONNECTION_OK; return PGRES_POLLING_OK; @@ -3300,9 +3300,6 @@ keep_going: /* We will come back to here until there is PQclear(res); termPQExpBuffer(&savedMessage); - /* We can release the address lists now. */ - release_all_addrinfo(conn); - /* * Finish reading any remaining messages before being * considered as ready. @@ -3639,32 +3636,18 @@ freePGconn(PGconn *conn) } /* - * release_all_addrinfo - * - free addrinfo of all hostconn elements. + * release_conn_addrinfo + * - Free any addrinfo list in the PGconn. */ - static void -release_all_addrinfo(PGconn *conn) +release_conn_addrinfo(PGconn *conn) { - if (conn->connhost != NULL) + if (conn->addrlist) { - int i; - - for (i = 0; i < conn->nconnhost; ++i) - { - int family = AF_UNSPEC; - -#ifdef HAVE_UNIX_SOCKETS - if (conn->connhost[i].type == CHT_UNIX_SOCKET) - family = AF_UNIX; -#endif - - pg_freeaddrinfo_all(family, - conn->connhost[i].addrlist); - conn->connhost[i].addrlist = NULL; - } + pg_freeaddrinfo_all(conn->addrlist_family, conn->addrlist); + conn->addrlist = NULL; + conn->addr_cur = NULL; /* for safety */ } - conn->addr_cur = NULL; } /* @@ -3723,7 +3706,7 @@ closePGconn(PGconn *conn) conn->xactStatus = PQTRANS_IDLE; pqClearAsyncResult(conn); /* deallocate result */ resetPQExpBuffer(&conn->errorMessage); - release_all_addrinfo(conn); + release_conn_addrinfo(conn); /* Reset all state obtained from server, too */ pqDropServerData(conn); diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index bc60373cb0..fb04c8c61d 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -312,7 +312,6 @@ typedef struct pg_conn_host char *password; /* password for this host, read from the * password file; NULL if not sought or not * found in password file. */ - struct addrinfo *addrlist; /* list of possible backend addresses */ } pg_conn_host; /* @@ -412,7 +411,9 @@ struct pg_conn /* Transient state needed while establishing connection */ bool try_next_addr; /* time to advance to next address/host? */ bool try_next_host; /* time to advance to next connhost[]? */ - struct addrinfo *addr_cur; /* backend address currently being tried */ + struct addrinfo *addrlist; /* list of addresses for current connhost */ + struct addrinfo *addr_cur; /* the one currently being tried */ + int addrlist_family; /* needed to know how to free addrlist */ PGSetenvStatusType setenv_state; /* for 2.0 protocol only */ const PQEnvironmentOption *next_eo; bool send_appname; /* okay to send application_name? */ -- 2.40.0