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@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
Import Notes
Reply to msg id not found: 430877A3.8010803@seznam.czReference msg id not found: 6BCB9D8A16AC4241919521715F4D8BCE0946A4@algol.sollentuna.seReference msg id not found: 4307A1B9.9090108@dunslane.netReference msg id not found: 430877A3.8010803@seznam.cz
Andrew Dunstan <andrew@dunslane.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@dunslane.net> writes:
I am bit worried about those HAVE_IPV6 ifdefs - they will prolly have
to be modified to C code under windowsNow 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@dunslane.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@dunslane.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
Attachments:
initdb-ip6.patchtext/x-patch; name=initdb-ip6.patchDownload
Index: src/bin/initdb/initdb.c
===================================================================
RCS file: /home/cvsmirror/pgsql/src/bin/initdb/initdb.c,v
retrieving revision 1.94
diff -c -r1.94 initdb.c
*** src/bin/initdb/initdb.c 2 Aug 2005 15:16:27 -0000 1.94
--- src/bin/initdb/initdb.c 21 Aug 2005 23:41:25 -0000
***************
*** 148,156 ****
static char *xstrdup(const char *s);
static char **replace_token(char **lines,
const char *token, const char *replacement);
- #ifndef HAVE_UNIX_SOCKETS
static char **filter_lines_with_token(char **lines, const char *token);
- #endif
static char **readfile(char *path);
static void writefile(char *path, char **lines);
static FILE *popen_check(const char *command, const char *mode);
--- 148,154 ----
***************
*** 330,336 ****
*
* a sort of poor man's grep -v
*/
- #ifndef HAVE_UNIX_SOCKETS
static char **
filter_lines_with_token(char **lines, const char *token)
{
--- 328,333 ----
***************
*** 351,357 ****
return result;
}
- #endif
/*
* get the lines from a text file
--- 348,353 ----
***************
*** 1157,1162 ****
--- 1153,1170 ----
char **conflines;
char repltok[100];
char path[MAXPGPATH];
+
+ struct addrinfo *gai_result;
+ struct addrinfo hints;
+
+ 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;
fputs(_("creating configuration files ... "), stdout);
fflush(stdout);
***************
*** 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,
--- 1218,1234 ----
conflines = replace_token(conflines,"@remove-line-for-nolocal@","");
#endif
! /*
! * runtime test for IPv6 support (previously compile time test).
! * this lets us build on hosts with IPv6 support and run
! * on hosts without, especially on Windows.
! *
! */
! if (getaddrinfo("::1", NULL, &hints, &gai_result) != 0)
! conflines = filter_lines_with_token(conflines,
! "@remove-line-for-no-ip6@");
! else
! conflines = replace_token(conflines,"@remove-line-for-no-ip6@","");
/* Replace default authentication methods */
conflines = replace_token(conflines,
Index: src/backend/libpq/pg_hba.conf.sample
===================================================================
RCS file: /home/cvsmirror/pgsql/src/backend/libpq/pg_hba.conf.sample,v
retrieving revision 1.59
diff -c -r1.59 pg_hba.conf.sample
*** src/backend/libpq/pg_hba.conf.sample 15 Aug 2005 02:40:25 -0000 1.59
--- src/backend/libpq/pg_hba.conf.sample 21 Aug 2005 23:33:30 -0000
***************
*** 67,71 ****
@remove-line-for-nolocal@local all all @authmethod@
# IPv4 local connections:
host all all 127.0.0.1/32 @authmethod@
! # IPv6 local connections:
! host all all ::1/128 @authmethod@
--- 67,71 ----
@remove-line-for-nolocal@local all all @authmethod@
# IPv4 local connections:
host all all 127.0.0.1/32 @authmethod@
! @remove-line-for-no-ip6@# IPv6 local connections:
! @remove-line-for-no-ip6@host all all ::1/128 @authmethod@
Andrew Dunstan <andrew@dunslane.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@dunslane.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
Attachments:
initdb-ip6.patch2text/plain; name=initdb-ip6.patch2Download
Index: src/bin/initdb/initdb.c
===================================================================
RCS file: /home/cvsmirror/pgsql/src/bin/initdb/initdb.c,v
retrieving revision 1.94
diff -c -r1.94 initdb.c
*** src/bin/initdb/initdb.c 2 Aug 2005 15:16:27 -0000 1.94
--- src/bin/initdb/initdb.c 22 Aug 2005 13:35:22 -0000
***************
*** 1157,1162 ****
--- 1157,1174 ----
char **conflines;
char repltok[100];
char path[MAXPGPATH];
+
+ struct addrinfo *gai_result;
+ struct addrinfo hints;
+
+ 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;
fputs(_("creating configuration files ... "), stdout);
fflush(stdout);
***************
*** 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,
--- 1222,1239 ----
conflines = replace_token(conflines,"@remove-line-for-nolocal@","");
#endif
!
! /*
! * runtime test for IPv6 support (previously compile time test).
! * this lets us build on hosts with IPv6 support and run
! * on hosts without, especially on Windows.
! *
! */
! if (getaddrinfo("::1", NULL, &hints, &gai_result) != 0)
! conflines = replace_token(conflines,
! "host all all ::1",
! "#host all all ::1");
!
/* Replace default authentication methods */
conflines = replace_token(conflines,
Andrew Dunstan <andrew@dunslane.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@dunslane.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