List of hostaddrs not supported
While testing libpq and GSS the other day, I was surprised by the
behavior of the host and hostaddr libpq options, if you specify a list
of hostnames.
I did this this, and it took me quite a while to figure out what was
going on:
$ psql "dbname=postgres hostaddr=::1 host=localhost,localhost user=krbtestuser" -c "SELECT 'hello'"
psql: GSSAPI continuation error: Unspecified GSS failure. Minor code may provide more information
GSSAPI continuation error: Server postgres/localhost,localhost@PG.EXAMPLE not found in Kerberos database
That was a pilot error; I specified a list of hostnames, but only one
hostaddr. But I would've expected to get a more helpful error, pointing
that out.
Some thoughts on this:
1. You cannot actually specify a list of hostaddrs. Trying to do so
gives error:
psql: could not translate host name "::1,::1" to address: Name or service not known
That error message is a bit inaccurate, in that it wasn't really a "host
name" that it tried to translate, but a raw address in string format.
That's even more confusing if you make the mistake that you specify
"hostaddr=localhost":
psql: could not translate host name "localhost" to address: Name or service not known
But in the first case, could we detect that there is a comma in the
string, and give an error along the lines of "list of hostaddr's not
supported". (Or better yet, support multiple hostaddrs)
2. The documentation is not very clear on the fact that multiple
hostaddr's is not supported. Nor what happens if you specify a single
hostaddr, but a list of hostnames. (The list of hostnames gets treated
as a single hostname, that's what.)
So, this is all quite confusing. I think we should support a list of
hostaddrs, to go with the list of hostnames. It seems like a strange
omission. Looking at the archives, it was mentioned a few times when
this was developed and reviewed, latest Takayuki Tsunakawa asked [1]/messages/by-id/0A3221C70F24FB45833433255569204D1F63FB5E@G01JPEXMBYT05 the
same question, but it was then forgotten about.
[1]: /messages/by-id/0A3221C70F24FB45833433255569204D1F63FB5E@G01JPEXMBYT05
/messages/by-id/0A3221C70F24FB45833433255569204D1F63FB5E@G01JPEXMBYT05
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Heikki Linnakangas <hlinnaka@iki.fi> writes:
So, this is all quite confusing. I think we should support a list of
hostaddrs, to go with the list of hostnames. It seems like a strange
omission.
+1, if it's not too large a patch. It could be argued that this is
a new feature and should wait for v11, but the behavior you describe
is weird enough that it could also be called a bug fix.
If it seems too hard to fix fully for v10, maybe we could insert
some checking code that just counts the number of entries in the
two lists and insists they match (which we'd keep later), plus
some code to reject >1 hostaddr (which would eventually go away).
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 8, 2017 at 4:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
So, this is all quite confusing. I think we should support a list of
hostaddrs, to go with the list of hostnames. It seems like a strange
omission. Looking at the archives, it was mentioned a few times when this
was developed and reviewed, latest Takayuki Tsunakawa asked [1] the same
question, but it was then forgotten about.
I didn't really forget about it; I just didn't think it seemed
important. There seemed to be a danger of scope creep, too. For
example, you could argue that multiplicity ought to also be permitted
for passwords. The status quo is that you can use different passwords
for different hosts if you specify the password via your .pgpass file,
but not if you include it in the connection string, and somebody could
argue that's weird and inconsistent. But if you allow multiple
passwords then maybe you ought to also allow multiple usernames. And
what do you do about the possibility that a password contains a
literal comma? And if you allow a different password for each host,
maybe you ought to allow a different sslcert, too, for people not
using passwords to authenticate. Maybe hostaddr is more
closely-related than any of that stuff, but I just made a judgement
call that host by itself was going to be a problem but host+port was
enough to make a credible minimal patch, and I didn't want to go
beyond what was absolutely required for fear of biting off more than I
could chew.
It doesn't seem like a problem to me if somebody else wants to extend
it to hostaddr, though. Whether that change belongs in v10 or v11 is
debatable. I would object to adding this as an open item with me as
the owner because doesn't seem to me to be a must-fix issue, but I
don't mind someone else doing the work.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
It doesn't seem like a problem to me if somebody else wants to extend
it to hostaddr, though. Whether that change belongs in v10 or v11 is
debatable. I would object to adding this as an open item with me as
the owner because doesn't seem to me to be a must-fix issue, but I
don't mind someone else doing the work.
If you want to define multiple-hostaddrs as a future feature, that
seems fine, but I think Heikki is describing actual bugs. The minimum
that I think needs to be done for v10 is to make libpq reject a hostaddr
string with the wrong number of entries (either different from the
host list, or different from 1).
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 8, 2017 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
It doesn't seem like a problem to me if somebody else wants to extend
it to hostaddr, though. Whether that change belongs in v10 or v11 is
debatable. I would object to adding this as an open item with me as
the owner because doesn't seem to me to be a must-fix issue, but I
don't mind someone else doing the work.If you want to define multiple-hostaddrs as a future feature, that
seems fine, but I think Heikki is describing actual bugs. The minimum
that I think needs to be done for v10 is to make libpq reject a hostaddr
string with the wrong number of entries (either different from the
host list, or different from 1).
Whatever you put in the hostaddr field - or any field other than host
and port - is one entry. There is no notion of a list of entries in
any other field, and no attempt to split any other field on a comma or
any other symbol. The fact that ::1,::1 looks to you like two entries
rather than a single malformed entry is just a misunderstanding on
your part, just like you'd be wrong if you thought that
password=foo,bar is a list of passwords rather than a password
containing a comma.
I think the argument is about whether I made the right decision when I
scoped the feature, not about whether there's a defect in the
implementation.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 8, 2017 at 8:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Whatever you put in the hostaddr field - or any field other than host
and port - is one entry. There is no notion of a list of entries in
any other field, and no attempt to split any other field on a comma or
any other symbol.
[...]
I think the argument is about whether I made the right decision when I
scoped the feature, not about whether there's a defect in the
implementation.
Implementation comes into play if the scope decision stands.
I have no immediate examples but it doesn't seem that we usually go to
great lengths to infer user intent and show hints based upon said
inference. But we also don't forbid doing so. So the question of whether
we should implement better error messages here seems to be in scope -
especially since we do allow for lists in some of the sibling fields.
These are already failing so I'd agree that explicit rejection isn't
necessary - the question seems restricted to usability. Though I suppose
we need to consider whether there is any problem with the current setup if
indeed our intended separator is also an allowable character - i.e., do we
want to future-proof the syntax by requiring quotes now?
David J.
On 06/08/2017 06:39 PM, David G. Johnston wrote:
These are already failing so I'd agree that explicit rejection isn't
necessary - the question seems restricted to usability. Though I suppose
we need to consider whether there is any problem with the current setup if
indeed our intended separator is also an allowable character - i.e., do we
want to future-proof the syntax by requiring quotes now?
Hmm, there is one problem with our current use of comma as a separator:
you cannot use a Unix-domain socket directory that has a comma in the
name, because it's interpreted as multiple hostnames. E.g. this doesn't
work:
psql "host=/tmp/dir,with,commas"
For hostnames, ports, and network addresses (hostaddr), a comma is not a
problem, as it's not a valid character in any of those.
I don't know if that was considered when this patch was developed. I
couldn't find a mention of this in the archives. But in any case, that's
quite orthogonal to the rest of this thread.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 9, 2017 at 3:22 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Hmm, there is one problem with our current use of comma as a separator:
you
cannot use a Unix-domain socket directory that has a comma in the name,
because it's interpreted as multiple hostnames. E.g. this doesn't work:psql "host=/tmp/dir,with,commas"
For hostnames, ports, and network addresses (hostaddr), a comma is not a
problem, as it's not a valid character in any of those.I don't know if that was considered when this patch was developed. I
couldn't find a mention of this in the archives. But in any case, that's
quite orthogonal to the rest of this thread.
I think this was found earlier [1]UDS with comma in name </messages/by-id/CAD__OujtKhMV9kQhQ2sgWV9EyzSv_Gwd7Kd=P1Lq+0z8xhW1RQ@mail.gmail.com>. But It appeared difficult to fix
without breaking other API's behavior [2]Fix for comma in UDS path </messages/by-id/CAD__Ouj-13zkmkm6CX7dW3UE+OaAHvaOB9Yx-bYj4yOHmsJ_FQ@mail.gmail.com>
[1]: UDS with comma in name </messages/by-id/CAD__OujtKhMV9kQhQ2sgWV9EyzSv_Gwd7Kd=P1Lq+0z8xhW1RQ@mail.gmail.com>
</messages/by-id/CAD__OujtKhMV9kQhQ2sgWV9EyzSv_Gwd7Kd=P1Lq+0z8xhW1RQ@mail.gmail.com>
[2]: Fix for comma in UDS path </messages/by-id/CAD__Ouj-13zkmkm6CX7dW3UE+OaAHvaOB9Yx-bYj4yOHmsJ_FQ@mail.gmail.com>
</messages/by-id/CAD__Ouj-13zkmkm6CX7dW3UE+OaAHvaOB9Yx-bYj4yOHmsJ_FQ@mail.gmail.com>
On 06/08/2017 06:18 PM, Robert Haas wrote:
On Thu, Jun 8, 2017 at 10:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
It doesn't seem like a problem to me if somebody else wants to extend
it to hostaddr, though. Whether that change belongs in v10 or v11 is
debatable. I would object to adding this as an open item with me as
the owner because doesn't seem to me to be a must-fix issue, but I
don't mind someone else doing the work.If you want to define multiple-hostaddrs as a future feature, that
seems fine, but I think Heikki is describing actual bugs. The minimum
that I think needs to be done for v10 is to make libpq reject a hostaddr
string with the wrong number of entries (either different from the
host list, or different from 1).Whatever you put in the hostaddr field - or any field other than host
and port - is one entry. There is no notion of a list of entries in
any other field, and no attempt to split any other field on a comma or
any other symbol. The fact that ::1,::1 looks to you like two entries
rather than a single malformed entry is just a misunderstanding on
your part, just like you'd be wrong if you thought that
password=foo,bar is a list of passwords rather than a password
containing a comma.I think the argument is about whether I made the right decision when I
scoped the feature, not about whether there's a defect in the
implementation.
Right. I think it's a usability fail as it is; it certainly fooled me.
We could make the error messages and documentation more clear. But even
better to allow multiple host addresses, so that it works as you'd expect.
I understand the slippery-slope argument that you might also want to
have different usernames etc. for different hosts, but it's confusing
that specifying a hostaddr changes the way the host-argument is
interpreted. In the worst case, if we let that stand, someone might
actually start to depend on that behavior. The other options don't have
that issue. And hostaddr is much more closely tied to specifying the
target to connect to, like host and port are.
I propose the attached patch, to allow a comma-separated list of
hostaddr's. It includes some refactoring of the code to parse
comma-separated lists, as it was getting a bit repetitive. It also fixes
a couple of minor issues in error messages, see commit message.
The patch doesn't include docs changes yet. I think we should add a new
sub-section, at the same level as the "33.1.1.1. Keyword/Value
Connection Strings" and "33.1.1.2. Connection URIs" sub-sections, to
explain some of the details we've discussed here. Something like:
---
33.1.1.3 Specifying Multiple Hosts
It is possible to specify multiple hosts to connect to, so that they are
tried in the order given. In the Keyword/Value syntax, the host,
hostaddr, and port options accept a comma-separated list of values. The
same number of elements should be given in each option, 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 applies to all the hosts.
In the connection URI syntax, you can list multiple host:port pairs
separated by commas, in the host component of the URI.
A single hostname can also translate to multiple network addresses. A
common example of this is that a host can have both an IPv4 and an IPv6
address.
When multiple hosts are specified, or when a single hostname is
translated to multiple addresses, all the hosts and addresses will be
tried in order, until one succeeds. If none of the hosts can be reached,
the connection fails. If a connection is established successfully, but
authentication fails, the remaining hosts in the list are not tried.
If a password file is used, you can have different passwords for
different hosts. All the other connection options are the same for every
host, it is not possible to e.g. specify a different username for
different hosts.
---
- Heikki
Attachments:
0001-Allow-multiple-hostaddrs-to-go-with-multiple-hostnam.patchinvalid/octet-stream; name=0001-Allow-multiple-hostaddrs-to-go-with-multiple-hostnam.patchDownload
From dd24faaf56a5c38389899f95885d8032ecac2143 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Jun 2017 13:26:44 +0300
Subject: [PATCH 1/1] Allow multiple hostaddrs to go with multiple hostnames.
XXX: Docs missing
Random other issues fixed while we're at it:
* In error message on connection failure, if multiple network addresses
were given as the host option, as in "host=127.0.0.1,127.0.0.2", the
error message printed the address twice.
* If there were many more ports than hostnames, the error message would
always claim that there was one port too many, even if there was more than
one. For example, if you gave 2 hostnames and 5 ports, the error message
claimed that you gave 2 hostnames and 3 ports.
Discussion: https://www.postgresql.org/message-id/10badbc6-4d5a-a769-623a-f7ada43e14dd@iki.fi
---
src/interfaces/libpq/fe-connect.c | 258 ++++++++++++++++++++++++--------------
src/interfaces/libpq/libpq-int.h | 3 +-
2 files changed, 168 insertions(+), 93 deletions(-)
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c1dfa5eb97..0ed6ec668f 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -809,6 +809,62 @@ connectOptions1(PGconn *conn, const char *conninfo)
}
/*
+ * Count the number of elements in a simple comma-separated list.
+ */
+static int
+count_comma_separated_elems(const char *input)
+{
+ int n;
+
+ n = 1;
+ for (; *input != '\0'; input++)
+ {
+ if (*input == ',')
+ n++;
+ }
+
+ return n;
+}
+
+/*
+ * Parse a simple comma-separated list.
+ *
+ * On each call, returns a malloc'd copy of the next element, and sets *more
+ * to indicate whether there are any more elements in the list after this,
+ * and updates *startptr to point to the next element, if any.
+ *
+ * On out of memory, returns NULL.
+ */
+static char *
+parse_comma_separated_list(char **startptr, bool *more)
+{
+ char *p;
+ char *s = *startptr;
+ char *e;
+ int len;
+
+ /*
+ * Search for the end of the current element; a comma or end-of-string
+ * acts as a terminator.
+ */
+ e = s;
+ while (*e != '\0' && *e != ',')
+ ++e;
+ *more = (*e == ',');
+
+ len = e - s;
+ p = (char *) malloc(sizeof(char) * (len + 1));
+ if (p)
+ {
+ memcpy(p, s, len);
+ p[len] = '\0';
+ }
+ *startptr = e + 1;
+
+ return p;
+}
+
+/*
* connectOptions2
*
* Compute derived connection options after absorbing all user-supplied info.
@@ -821,21 +877,16 @@ connectOptions2(PGconn *conn)
{
/*
* Allocate memory for details about each host to which we might possibly
- * try to connect. If pghostaddr is set, we're only going to try to
- * connect to that one particular address. If it's not, we'll use pghost,
- * which may contain multiple, comma-separated names.
+ * try to connect. For that, count the number of elements in the hostaddr
+ * or host options. If neither is given, assume one host.
*/
- conn->nconnhost = 1;
conn->whichhost = 0;
- if ((conn->pghostaddr == NULL || conn->pghostaddr[0] == '\0')
- && conn->pghost != NULL)
- {
- char *s;
-
- for (s = conn->pghost; *s != '\0'; ++s)
- if (*s == ',')
- conn->nconnhost++;
- }
+ if (conn->pghostaddr && conn->pghostaddr[0] != '\0')
+ conn->nconnhost = count_comma_separated_elems(conn->pghostaddr);
+ else if (conn->pghost && conn->pghost[0] != '\0')
+ conn->nconnhost = count_comma_separated_elems(conn->pghost);
+ else
+ conn->nconnhost = 1;
conn->connhost = (pg_conn_host *)
calloc(conn->nconnhost, sizeof(pg_conn_host));
if (conn->connhost == NULL)
@@ -847,51 +898,67 @@ connectOptions2(PGconn *conn)
*/
if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0')
{
- conn->connhost[0].host = strdup(conn->pghostaddr);
- if (conn->connhost[0].host == NULL)
- goto oom_error;
- conn->connhost[0].type = CHT_HOST_ADDRESS;
+ int i;
+ char *s = conn->pghostaddr;
+ bool more = true;
+
+ for (i = 0; i < conn->nconnhost && more; i++)
+ {
+ conn->connhost[i].hostaddr = parse_comma_separated_list(&s, &more);
+ if (conn->connhost[i].hostaddr == NULL)
+ goto oom_error;
+
+ conn->connhost[i].type = CHT_HOST_ADDRESS;
+ }
+
+ /*
+ * If hostaddr was given, the array was allocated according to the
+ * number of elements in the hostaddr list, so it really should be the
+ * right size.
+ */
+ Assert(!more);
+ Assert(i == conn->nconnhost);
}
- else if (conn->pghost != NULL && conn->pghost[0] != '\0')
+
+ if (conn->pghost != NULL && conn->pghost[0] != '\0')
{
- int i = 0;
+ int i;
char *s = conn->pghost;
+ bool more = true;
- while (1)
+ for (i = 0; i < conn->nconnhost && more; i++)
{
- char *e = s;
-
- /*
- * Search for the end of the current hostname; a comma or
- * end-of-string acts as a terminator.
- */
- while (*e != '\0' && *e != ',')
- ++e;
-
- /* Copy the hostname whose bounds we just identified. */
- conn->connhost[i].host =
- (char *) malloc(sizeof(char) * (e - s + 1));
+ conn->connhost[i].host = parse_comma_separated_list(&s, &more);
if (conn->connhost[i].host == NULL)
goto oom_error;
- memcpy(conn->connhost[i].host, s, e - s);
- conn->connhost[i].host[e - s] = '\0';
/* Identify the type of host. */
- conn->connhost[i].type = CHT_HOST_NAME;
+ if (conn->pghostaddr == NULL || conn->pghostaddr[0] == '\0')
+ {
+ conn->connhost[i].type = CHT_HOST_NAME;
#ifdef HAVE_UNIX_SOCKETS
- if (is_absolute_path(conn->connhost[i].host))
- conn->connhost[i].type = CHT_UNIX_SOCKET;
+ if (is_absolute_path(conn->connhost[i].host))
+ conn->connhost[i].type = CHT_UNIX_SOCKET;
#endif
-
- /* Prepare to find the next host (if any). */
- if (*e == '\0')
- break;
- s = e + 1;
- i++;
+ }
+ }
+ if (more || i != conn->nconnhost)
+ {
+ conn->status = CONNECTION_BAD;
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("could not match %d host names to %d hostaddrs\n"),
+ count_comma_separated_elems(conn->pghost), conn->nconnhost);
+ return false;
}
}
- else
+
+ /*
+ * If neither host or hostaddr options was given, connect to default host.
+ */
+ if ((conn->pghostaddr == NULL || conn->pghostaddr[0] == '\0') &&
+ (conn->pghost == NULL || conn->pghost[0] == '\0'))
{
+ Assert(conn->nconnhost == 1);
#ifdef HAVE_UNIX_SOCKETS
conn->connhost[0].host = strdup(DEFAULT_PGSOCKET_DIR);
conn->connhost[0].type = CHT_UNIX_SOCKET;
@@ -908,54 +975,36 @@ connectOptions2(PGconn *conn)
*/
if (conn->pgport != NULL && conn->pgport[0] != '\0')
{
- int i = 0;
+ int i;
char *s = conn->pgport;
- int nports = 1;
+ bool more = true;
- for (i = 0; i < conn->nconnhost; ++i)
+ for (i = 0; i < conn->nconnhost && more; i++)
{
- char *e = s;
-
- /* Search for the end of the current port number. */
- while (*e != '\0' && *e != ',')
- ++e;
+ conn->connhost[i].port = parse_comma_separated_list(&s, &more);
+ if (conn->connhost[i].port == NULL)
+ goto oom_error;
+ }
- /*
- * If we found a port number of non-zero length, copy it.
- * Otherwise, insert the default port number.
- */
- if (e > s)
+ /*
+ * If exactly one port was given, use it for every host. Otherwise,
+ * there must be exactly as many ports as there were hosts.
+ */
+ if (i == 1 && !more)
+ {
+ for (i = 1; i < conn->nconnhost; i++)
{
- conn->connhost[i].port =
- (char *) malloc(sizeof(char) * (e - s + 1));
+ conn->connhost[i].port = strdup(conn->connhost[0].port);
if (conn->connhost[i].port == NULL)
goto oom_error;
- memcpy(conn->connhost[i].port, s, e - s);
- conn->connhost[i].port[e - s] = '\0';
- }
-
- /*
- * Move on to the next port number, unless there are no more. (If
- * only one part number is specified, we reuse it for every host.)
- */
- if (*e != '\0')
- {
- s = e + 1;
- ++nports;
}
}
-
- /*
- * If multiple ports were specified, there must be exactly as many
- * ports as there were hosts. Otherwise, we do not know how to match
- * them up.
- */
- if (nports != 1 && nports != conn->nconnhost)
+ else if (more || i != conn->nconnhost)
{
conn->status = CONNECTION_BAD;
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not match %d port numbers to %d hosts\n"),
- nports, conn->nconnhost);
+ count_comma_separated_elems(conn->pgport), conn->nconnhost);
return false;
}
}
@@ -1029,8 +1078,8 @@ connectOptions2(PGconn *conn)
char *pwhost = conn->connhost[i].host;
if (conn->connhost[i].type == CHT_HOST_ADDRESS &&
- conn->pghost != NULL && conn->pghost[0] != '\0')
- pwhost = conn->pghost;
+ conn->connhost[i].host != NULL && conn->connhost[i].host != '\0')
+ pwhost = conn->connhost[i].hostaddr;
conn->connhost[i].password =
passwordFromFile(pwhost,
@@ -1380,8 +1429,8 @@ connectFailureMessage(PGconn *conn, int errorno)
* Optionally display the network address with the hostname. This is
* useful to distinguish between IPv4 and IPv6 connections.
*/
- if (conn->pghostaddr != NULL)
- strlcpy(host_addr, conn->pghostaddr, NI_MAXHOST);
+ if (conn->connhost[conn->whichhost].type == CHT_HOST_ADDRESS)
+ strlcpy(host_addr, conn->connhost[conn->whichhost].hostaddr, NI_MAXHOST);
else if (addr->ss_family == AF_INET)
{
if (inet_net_ntop(AF_INET,
@@ -1404,7 +1453,10 @@ connectFailureMessage(PGconn *conn, int errorno)
strcpy(host_addr, "???");
/* To which host and port were we actually connecting? */
- displayed_host = conn->connhost[conn->whichhost].host;
+ if (conn->connhost[conn->whichhost].type == CHT_HOST_ADDRESS)
+ displayed_host = conn->connhost[conn->whichhost].hostaddr;
+ else
+ displayed_host = conn->connhost[conn->whichhost].host;
displayed_port = conn->connhost[conn->whichhost].port;
if (displayed_port == NULL || displayed_port[0] == '\0')
displayed_port = DEF_PGPORT_STR;
@@ -1414,8 +1466,8 @@ connectFailureMessage(PGconn *conn, int errorno)
* 'host' was missing or does not match our lookup, display the
* looked-up IP address.
*/
- if ((conn->pghostaddr == NULL) &&
- (conn->pghost == NULL || strcmp(conn->pghost, host_addr) != 0))
+ if (conn->connhost[conn->whichhost].type != CHT_HOST_ADDRESS &&
+ strcmp(displayed_host, host_addr) != 0)
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("could not connect to server: %s\n"
"\tIs the server running on host \"%s\" (%s) and accepting\n"
@@ -1651,7 +1703,7 @@ connectDBStart(PGconn *conn)
hint.ai_family = AF_UNSPEC;
/* Figure out the port number we're going to use. */
- if (ch->port == NULL)
+ if (ch->port == NULL || ch->port[0] == '\0')
thisport = DEF_PGPORT;
else
{
@@ -1680,7 +1732,7 @@ connectDBStart(PGconn *conn)
case CHT_HOST_ADDRESS:
hint.ai_flags = AI_NUMERICHOST;
- ret = pg_getaddrinfo_all(ch->host, portstr, &hint, &ch->addrlist);
+ 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"),
@@ -3024,6 +3076,9 @@ keep_going: /* We will come back to here until there is
}
case CONNECTION_CHECK_WRITABLE:
{
+ const char *displayed_host;
+ const char *displayed_port;
+
if (!saveErrorMessage(conn, &savedMessage))
goto error_return;
@@ -3050,6 +3105,17 @@ keep_going: /* We will come back to here until there is
val = PQgetvalue(res, 0, 0);
if (strncmp(val, "on", 2) == 0)
{
+ const char *displayed_host;
+ const char *displayed_port;
+
+ if (conn->connhost[conn->whichhost].type == CHT_HOST_ADDRESS)
+ displayed_host = conn->connhost[conn->whichhost].hostaddr;
+ else
+ displayed_host = conn->connhost[conn->whichhost].host;
+ displayed_port = conn->connhost[conn->whichhost].port;
+ if (displayed_port == NULL || displayed_port[0] == '\0')
+ displayed_port = DEF_PGPORT_STR;
+
PQclear(res);
restoreErrorMessage(conn, &savedMessage);
@@ -3058,8 +3124,7 @@ keep_going: /* We will come back to here until there is
libpq_gettext("could not make a writable "
"connection to server "
"\"%s:%s\"\n"),
- conn->connhost[conn->whichhost].host,
- conn->connhost[conn->whichhost].port);
+ displayed_host, displayed_port);
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
pqDropConnection(conn, true);
@@ -3096,11 +3161,18 @@ keep_going: /* We will come back to here until there is
if (res)
PQclear(res);
restoreErrorMessage(conn, &savedMessage);
+
+ if (conn->connhost[conn->whichhost].type == CHT_HOST_ADDRESS)
+ displayed_host = conn->connhost[conn->whichhost].hostaddr;
+ else
+ displayed_host = conn->connhost[conn->whichhost].host;
+ displayed_port = conn->connhost[conn->whichhost].port;
+ if (displayed_port == NULL || displayed_port[0] == '\0')
+ displayed_port = DEF_PGPORT_STR;
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("test \"SHOW transaction_read_only\" failed "
" on \"%s:%s\"\n"),
- conn->connhost[conn->whichhost].host,
- conn->connhost[conn->whichhost].port);
+ displayed_host, displayed_port);
conn->status = CONNECTION_OK;
sendTerminateConn(conn);
pqDropConnection(conn, true);
@@ -3333,6 +3405,8 @@ freePGconn(PGconn *conn)
{
if (conn->connhost[i].host != NULL)
free(conn->connhost[i].host);
+ if (conn->connhost[i].hostaddr != NULL)
+ free(conn->connhost[i].hostaddr);
if (conn->connhost[i].port != NULL)
free(conn->connhost[i].port);
if (conn->connhost[i].password != NULL)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 335568b790..ab2ded97f1 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -304,8 +304,9 @@ typedef enum pg_conn_host_type
*/
typedef struct pg_conn_host
{
- char *host; /* host name or address, or socket path */
pg_conn_host_type type; /* type of host */
+ char *host; /* host name or socket path */
+ char *hostaddr; /* host address */
char *port; /* port number for this host; if not NULL,
* overrides the PGConn's pgport */
char *password; /* password for this host, read from the
--
2.11.0
On Fri, Jun 9, 2017 at 6:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Right. I think it's a usability fail as it is; it certainly fooled me. We
could make the error messages and documentation more clear. But even better
to allow multiple host addresses, so that it works as you'd expect.
Sure, I don't have a problem with that. I guess part of the point of
beta releases is to correct things that don't turn out to be as smart
as we thought they were, and this seems to be an example of that.
I understand the slippery-slope argument that you might also want to have
different usernames etc. for different hosts, but it's confusing that
specifying a hostaddr changes the way the host-argument is interpreted. In
the worst case, if we let that stand, someone might actually start to depend
on that behavior. The other options don't have that issue. And hostaddr is
much more closely tied to specifying the target to connect to, like host and
port are.
Yeah, I'm not objecting to your changes, just telling you what my
chain of reasoning was.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 8, 2017 at 1:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
While testing libpq and GSS the other day, I was surprised by the behavior
of the host and hostaddr libpq options, if you specify a list of hostnames.I did this this, and it took me quite a while to figure out what was going
on:$ psql "dbname=postgres hostaddr=::1 host=localhost,localhost
user=krbtestuser" -c "SELECT 'hello'"
psql: GSSAPI continuation error: Unspecified GSS failure. Minor code may
provide more information
GSSAPI continuation error: Server postgres/localhost,localhost@PG.EXAMPLE
not found in Kerberos databaseThat was a pilot error; I specified a list of hostnames, but only one
hostaddr. But I would've expected to get a more helpful error, pointing
that out.Some thoughts on this:
1. You cannot actually specify a list of hostaddrs. Trying to do so gives
error:psql: could not translate host name "::1,::1" to address: Name or service
not known
That error message is a bit inaccurate, in that it wasn't really a "host
name" that it tried to translate, but a raw address in string format.
That's even more confusing if you make the mistake that you specify
"hostaddr=localhost":
Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:
fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this function
gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)
Cheers,
Jeff
Jeff Janes <jeff.janes@gmail.com> writes:
Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:
fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this function
Me too ...
gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)
... which is not surprising since we're using the same compiler.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/09/2017 05:47 PM, Jeff Janes wrote:
Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this functiongcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)
Oh. Apparently that version of gcc doesn't take it for granted that the
switch-statement covers all the possible cases. I've added a dummy
initialization, to silence it. Thanks, and let me know if it didn't help.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 9, 2017 at 11:52 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 06/09/2017 05:47 PM, Jeff Janes wrote:
Your commit to fix this part, 76b11e8a43eca4612d, is giving me compiler
warnings:fe-connect.c: In function 'connectDBStart':
fe-connect.c:1625: warning: 'ret' may be used uninitialized in this
functiongcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)
Oh. Apparently that version of gcc doesn't take it for granted that the
switch-statement covers all the possible cases. I've added a dummy
initialization, to silence it. Thanks, and let me know if it didn't help.
It worked. Thanks.
Jeff
On 06/09/2017 04:26 PM, Robert Haas wrote:
On Fri, Jun 9, 2017 at 6:36 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
Right. I think it's a usability fail as it is; it certainly fooled me. We
could make the error messages and documentation more clear. But even better
to allow multiple host addresses, so that it works as you'd expect.Sure, I don't have a problem with that. I guess part of the point of
beta releases is to correct things that don't turn out to be as smart
as we thought they were, and this seems to be an example of that.
I just remembered that this was still pending. I made the documentation
changes, and committed this patch now.
We're uncomfortably close to wrapping the next beta, later today, but I
think it's better to get this into the hands of people testing this,
rather than wait for the next beta. I think the risk of breaking
something that used to work is small.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
2017-07-10 12:30 GMT+03:00 Heikki Linnakangas <hlinnaka@iki.fi>:
I just remembered that this was still pending. I made the documentation
changes, and committed this patch now.We're uncomfortably close to wrapping the next beta, later today, but I
think it's better to get this into the hands of people testing this, rather
than wait for the next beta. I think the risk of breaking something that
used to work is small.
I get this warning during compilation using gcc 7.1.1 20170621:
fe-connect.c:1100:61: warning: comparison between pointer and zero
character constant [-Wpointer-compare]
conn->connhost[i].host != NULL && conn->connhost[i].host != '\0')
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
On 07/10/2017 01:47 PM, Arthur Zakirov wrote:
Hello,
2017-07-10 12:30 GMT+03:00 Heikki Linnakangas <hlinnaka@iki.fi>:
I just remembered that this was still pending. I made the documentation
changes, and committed this patch now.We're uncomfortably close to wrapping the next beta, later today, but I
think it's better to get this into the hands of people testing this, rather
than wait for the next beta. I think the risk of breaking something that
used to work is small.I get this warning during compilation using gcc 7.1.1 20170621:
fe-connect.c:1100:61: warning: comparison between pointer and zero
character constant [-Wpointer-compare]
conn->connhost[i].host != NULL && conn->connhost[i].host != '\0')
Thanks, fixed! That check for empty hostname was indeed wrong.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers