Thread: [HACKERS] 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] the same question, but it was then forgotten about. [1] https://www.postgresql.org/message-id/0A3221C70F24FB45833433255569204D1F63FB5E%40G01JPEXMBYT05 - Heikki
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
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
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
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
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
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]. But It appeared difficult to fix without breaking other API's behavior [2]
> 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]. But It appeared difficult to fix without breaking other API's behavior [2]
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 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":
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
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 function > > gcc 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
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 function
gcc 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
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')
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