Re: [HACKERS] List of hostaddrs not supported

[Heikki Linnakangas]

On 07/10/2017 01:47 PM, Arthur Zakirov wrote:

Hello,

2017-07-10 12:30 GMT+03:00 Heikki Linnakangas :



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

Re: [HACKERS] List of hostaddrs not supported

[Arthur Zakirov]

Hello,

2017-07-10 12:30 GMT+03:00 Heikki Linnakangas :
>
>
> 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

Re: [HACKERS] List of hostaddrs not supported

[Heikki Linnakangas]

On 06/09/2017 04:26 PM, Robert Haas wrote:

On Fri, Jun 9, 2017 at 6:36 AM, Heikki Linnakangas  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

Re: [HACKERS] List of hostaddrs not supported

[Jeff Janes]

On Fri, Jun 9, 2017 at 11:52 AM, Heikki Linnakangas  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

Re: [HACKERS] List of hostaddrs not supported

[Heikki Linnakangas]

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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] List of hostaddrs not supported

[Tom Lane]

Jeff Janes  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

Re: [HACKERS] List of hostaddrs not supported

[Jeff Janes]

On Thu, Jun 8, 2017 at 1:36 AM, Heikki Linnakangas  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

Re: [HACKERS] List of hostaddrs not supported

[Robert Haas]

On Fri, Jun 9, 2017 at 6:36 AM, Heikki Linnakangas  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

Re: [HACKERS] List of hostaddrs not supported

[Heikki Linnakangas]

On 06/08/2017 06:18 PM, Robert Haas wrote:

On Thu, Jun 8, 2017 at 10:50 AM, Tom Lane  wrote:

Robert Haas  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



0001-Allow-multiple-hostaddrs-to-go-with-multiple-hostnam.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] List of hostaddrs not supported

[Mithun Cy]

On Fri, Jun 9, 2017 at 3:22 PM, Heikki Linnakangas  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

[2] Fix for comma in UDS path

Re: [HACKERS] List of hostaddrs not supported

[Heikki Linnakangas]

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

Re: [HACKERS] List of hostaddrs not supported

[David G. Johnston]

On Thu, Jun 8, 2017 at 8:18 AM, Robert Haas  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.

Re: [HACKERS] List of hostaddrs not supported

[Robert Haas]

On Thu, Jun 8, 2017 at 10:50 AM, Tom Lane  wrote:
> Robert Haas  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

Re: [HACKERS] List of hostaddrs not supported

[Tom Lane]

Robert Haas  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

Re: [HACKERS] List of hostaddrs not supported

[Robert Haas]

On Thu, Jun 8, 2017 at 4:36 AM, Heikki Linnakangas  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

Re: [HACKERS] List of hostaddrs not supported

[Tom Lane]

Heikki Linnakangas  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