Horton

Thread

Commits

  1. Fix check for empty hostname.

  2. Allow multiple hostaddrs to go with multiple hostnames.

  3. Silence warning about uninitialized 'ret' variable on some compilers.

  4. Give a better error message on invalid hostaddr option.

  1. List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-08T08:36:38Z

    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

  2. Re: List of hostaddrs not supported

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-08T13:49:10Z

    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

  3. Re: List of hostaddrs not supported

    Robert Haas <robertmhaas@gmail.com> — 2017-06-08T14:46:31Z

    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

  4. Re: List of hostaddrs not supported

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-08T14:50:26Z

    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

  5. Re: List of hostaddrs not supported

    Robert Haas <robertmhaas@gmail.com> — 2017-06-08T15:18:28Z

    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

  6. Re: List of hostaddrs not supported

    David G. Johnston <david.g.johnston@gmail.com> — 2017-06-08T15:39:06Z

    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.

  7. Re: List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-09T09:52:02Z

    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

  8. Re: List of hostaddrs not supported

    Mithun Cy <mithun.cy@enterprisedb.com> — 2017-06-09T10:22:20Z

    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]

[1] UDS with comma in name
<https://www.postgresql.org/message-id/CAD__OujtKhMV9kQhQ2sgWV9EyzSv_Gwd7Kd%3DP1Lq%2B0z8xhW1RQ%40mail.gmail.com>
[2] Fix for comma in UDS path
<https://www.postgresql.org/message-id/CAD__Ouj-13zkmkm6CX7dW3UE%2BOaAHvaOB9Yx-bYj4yOHmsJ_FQ%40mail.gmail.com>

  9. Re: List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-09T10:36:07Z

    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

  10. Re: List of hostaddrs not supported

    Robert Haas <robertmhaas@gmail.com> — 2017-06-09T13:26:19Z

    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

  11. Re: List of hostaddrs not supported

    Jeff Janes <jeff.janes@gmail.com> — 2017-06-09T14:47:37Z

    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

  12. Re: List of hostaddrs not supported

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-09T18:51:49Z

    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

  13. Re: List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-06-09T18:52:46Z

    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

  14. Re: List of hostaddrs not supported

    Jeff Janes <jeff.janes@gmail.com> — 2017-06-09T19:45:17Z

    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

  15. Re: List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-07-10T09:30:47Z

    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

  16. Re: List of hostaddrs not supported

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2017-07-10T10:47:00Z

    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

  17. Re: List of hostaddrs not supported

    Heikki Linnakangas <hlinnaka@iki.fi> — 2017-07-10T12:32:37Z

    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