Re: PostgreSQL 8.0.3 and Ipv6

[adding -hackers to discussion]

[getaddrinfo and friends are broken on some versions of windows]

Maggnus Hagander wrote:

>That definitly means it's broken. We need the same binary to run wether
>you have it or not - at least if we want it to be included in the
>precompiled binaries by the installer. That means we have to load the
>function with LoadLibrary / GetProcAddress, to check it at runtime.
>Yuck.
>
>
>

Petr Jelinek wrote:

> Andrew Dunstan wrote:
>
>>
>> Yep. I don't think we have much choice. The upside is that we can
>> let the configure test stay as is and not worry about it further.
>> Just put some ifdef''d code in src/port/getaddrinfo.c. Chuck McDevitt
>> kindly said he will try next week to produce a patch.
>>
>
> I am glad Chuck took it because I wouldn't be able to do it in
> reasonable time due to some probles in my real life.
>
> I am bit worried about those HAVE_IPV6 ifdefs - they will prolly have
> to be modified to C code under windows
>

Now I consider it you might be right. Here's a list of those places:

[andrew(at)alphonso src]$ grep -rl HAVE_IP .
./include/pg_config.h.in
./include/libpq/ip.h
./include/pg_config.h
./bin/initdb/initdb.c
./Makefile.global.in
./backend/libpq/pqcomm.c
./backend/libpq/ip.c
./backend/libpq/hba.c
./backend/utils/adt/pgstatfuncs.c
./backend/utils/adt/network.c
./Makefile.global
./interfaces/libpq/ip.c
./port/getaddrinfo.c

Can we even get this done for 8.1, or is it too late? If it's too late
we need to document heavily that we do not (fully) support IPv6 on
Windows yet.

Can someone please try running a build from CVS tip made on a modern box
(W2k3 or XP >= SP1 I believe) on a non-modern box (e.g. W2k) and see if
anything blows up? If it does then we either have to finish this work
now or revert the config file changes, I think.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>> I am bit worried about those HAVE_IPV6 ifdefs - they will prolly have
>> to be modified to C code under windows

> Now I consider it you might be right. Here's a list of those places:
> [lots]

You should not have to touch the HAVE_IPV6 code --- if you think you
do, you're misunderstanding the problem. The IPV6 code was designed
to work even if the local kernel does not understand IPV6 (of course
you don't actually get any IPV6 connectivity, but nothing breaks).
It should be possible to handle Windows the same way.

> Can we even get this done for 8.1, or is it too late?

Considering that this is a new feature that we didn't have in 8.0,
anything more than a very localized tweak is not going to be accepted
for 8.1.

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>>I am bit worried about those HAVE_IPV6 ifdefs - they will prolly have
>>>to be modified to C code under windows
>>>
>>>
>
>
>
>>Now I consider it you might be right. Here's a list of those places:
>>[lots]
>>
>>
>
>You should not have to touch the HAVE_IPV6 code --- if you think you
>do, you're misunderstanding the problem. The IPV6 code was designed
>to work even if the local kernel does not understand IPV6 (of course
>you don't actually get any IPV6 connectivity, but nothing breaks).
>It should be possible to handle Windows the same way.
>
>

Ok, looked at these more closely.

The one place that very slightly bothers me is the ::1 line in
pg_hba.conf. The fact that it comes last in the default config is its
saving grace - it won't ever be reached by a passing connection. I think
at least, though, we should put a warning comment line in front of it,
to the effect that if they see 'LOG: invalid IP address "::1"' in the
log or a connection message like 'FATAL: missing or erroneous
pg_hba.conf file' they probably need to comment the line out.

I agree that most of the others don't matter (most are there just for
case branches for AF_INET6).

>
>
>>Can we even get this done for 8.1, or is it too late?
>>
>>
>
>Considering that this is a new feature that we didn't have in 8.0,
>anything more than a very localized tweak is not going to be accepted
>for 8.1.
>
>
>
>

Apart from pg_hba.conf.sample (if you agree with the above), it looks
like just port/getaddrinfo.c will need tweaking.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> The one place that very slightly bothers me is the ::1 line in
> pg_hba.conf. The fact that it comes last in the default config is its
> saving grace - it won't ever be reached by a passing connection. I think
> at least, though, we should put a warning comment line in front of it,

If you like, you can improve initdb to comment that line out if
getaddrinfo chokes on "::1", rather than believing HAVE_IPV6.

regards, tom lane

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>The one place that very slightly bothers me is the ::1 line in
>>pg_hba.conf. The fact that it comes last in the default config is its
>>saving grace - it won't ever be reached by a passing connection. I think
>>at least, though, we should put a warning comment line in front of it,
>>
>>
>
>If you like, you can improve initdb to comment that line out if
>getaddrinfo chokes on "::1", rather than believing HAVE_IPV6.
>
>
>
>

Good idea. Here's a patch for that. Rather than commenting it out I used
the slightly newer initdb facility to remove it and the associated
comment line altogether.

Passes "make check" on my linux box.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Tom Lane wrote:
>> If you like, you can improve initdb to comment that line out if
>> getaddrinfo chokes on "::1", rather than believing HAVE_IPV6.

> Good idea. Here's a patch for that. Rather than commenting it out I used
> the slightly newer initdb facility to remove it and the associated
> comment line altogether.

Hm, is that really better than just commenting it out? Particularly
on Windows, where we could imagine someone installing a newer version
of the relevant DLL and then wanting to use IPv6. Seems to me that
leaving the line present but commented out is the right thing, because
it documents how things should look for IPv6.

regards, tom lane

Andrew Dunstan wrote:
> Can someone please try running a build from CVS tip made on a modern box
> (W2k3 or XP >= SP1 I believe) on a non-modern box (e.g. W2k) and see if
> anything blows up? If it does then we either have to finish this work
> now or revert the config file changes, I think.

W2k and XP SP1 tested, builds and passes make check (except rules but
thats just different sorting of output in my locale and I always had
this problem - wiech is after wieck because 'ch' is character after 'h'
in my alphabet).

--
Regards
Petr Jelinek (PJMODOS)

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Tom Lane wrote:
>>
>>
>>>If you like, you can improve initdb to comment that line out if
>>>getaddrinfo chokes on "::1", rather than believing HAVE_IPV6.
>>>
>>>
>
>
>
>>Good idea. Here's a patch for that. Rather than commenting it out I used
>>the slightly newer initdb facility to remove it and the associated
>>comment line altogether.
>>
>>
>
>Hm, is that really better than just commenting it out? Particularly
>on Windows, where we could imagine someone installing a newer version
>of the relevant DLL and then wanting to use IPv6. Seems to me that
>leaving the line present but commented out is the right thing, because
>it documents how things should look for IPv6.
>
>
>
>

Seemed to me slightly less potentially confusing, but I don't feel
strongly. Try this instead if you prefer.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Try this instead if you prefer.

I thought this was a little bit too trusting about there being a
getaddrinfo to probe, so I tightened it up as per the attached
applied patch.

Where are we at this point on the Windows/IPv6 issue --- are there
more fixes to come, or is it done?

regards, tom lane

*** src/bin/initdb/initdb.c.orig Tue Aug 2 11:16:27 2005
--- src/bin/initdb/initdb.c Mon Aug 22 12:24:42 2005
***************
*** 54,66 ****
#include <unistd.h>
#include <locale.h>
#include <signal.h>
- #include <errno.h>
#ifdef HAVE_LANGINFO_H
#include <langinfo.h>
#endif

#include "libpq/pqsignal.h"
#include "mb/pg_wchar.h"
#include "getopt_long.h"

#ifndef HAVE_INT_OPTRESET
--- 54,66 ----
#include <unistd.h>
#include <locale.h>
#include <signal.h>
#ifdef HAVE_LANGINFO_H
#include <langinfo.h>
#endif

#include "libpq/pqsignal.h"
#include "mb/pg_wchar.h"
+ #include "getaddrinfo.h"
#include "getopt_long.h"

#ifndef HAVE_INT_OPTRESET
***************
*** 1210,1220 ****
conflines = replace_token(conflines,"@remove-line-for-nolocal@","");
#endif

! #ifndef HAVE_IPV6
conflines = replace_token(conflines,
"host all all ::1",
"#host all all ::1");
! #endif

/* Replace default authentication methods */
conflines = replace_token(conflines,
--- 1210,1251 ----
conflines = replace_token(conflines,"@remove-line-for-nolocal@","");
#endif

! #if defined(HAVE_IPV6) && defined(HAVE_STRUCT_ADDRINFO) && defined(HAVE_GETADDRINFO)
! /*
! * Probe to see if there is really any platform support for IPv6, and
! * comment out the relevant pg_hba line if not. This avoids runtime
! * warnings if getaddrinfo doesn't actually cope with IPv6. Particularly
! * useful on Windows, where executables built on a machine with IPv6
! * may have to run on a machine without.
! *
! * We don't bother with testing if we aren't using the system version
! * of getaddrinfo, since we know our own version doesn't do IPv6.
! */
! {
! struct addrinfo *gai_result;
! struct addrinfo hints;
!
! /* for best results, this code should match parse_hba() */
! hints.ai_flags = AI_NUMERICHOST;
! hints.ai_family = PF_UNSPEC;
! hints.ai_socktype = 0;
! hints.ai_protocol = 0;
! hints.ai_addrlen = 0;
! hints.ai_canonname = NULL;
! hints.ai_addr = NULL;
! hints.ai_next = NULL;
!
! if (getaddrinfo("::1", NULL, &hints, &gai_result) != 0)
! conflines = replace_token(conflines,
! "host all all ::1",
! "#host all all ::1");
! }
! #else /* !HAVE_IPV6 etc */
! /* If we didn't compile IPV6 support at all, always comment it out */
conflines = replace_token(conflines,
"host all all ::1",
"#host all all ::1");
! #endif /* HAVE_IPV6 etc */

/* Replace default authentication methods */
conflines = replace_token(conflines,

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>Try this instead if you prefer.
>>
>>
>
>I thought this was a little bit too trusting about there being a
>getaddrinfo to probe, so I tightened it up as per the attached
>applied patch.
>
>Where are we at this point on the Windows/IPv6 issue --- are there
>more fixes to come, or is it done?
>
>

Not done yet. One thing left.

The idea was that we would put dynamic testing of properly working
getaddrinfo and friends on Windows into our getaddrinfo.c. That would
be the "local tweak" you mentioned previously ;-)

In consequence of that plan, I think we would need to remove "&&
defined(HAVE_GETADDRINFO)" from your applied patch and let it fall
through to our homegrown getaddrinfo if there isn't one. On most such
platforms it would fail, consuming a few more millisecs, but with
Windows with the expected patch it could pass.

(I know it's a muddle - I can't think how we came not to do IPv6 for
Windows in 8.0, or at the very least put it on the TODO list.)

cheers

andrew