Thread: Fix Windows socket error checking for MinGW
I started a thread on the general list so read that for more info. http://www.postgresql.org/message-id/520A6E55.40901@cchtml.com I'm also going to submit the patch to CommitFest. Thanks, Michael
Attachment
On Fri, Aug 16, 2013 at 06:56:45PM -0500, Michael Cronenworth wrote: > I started a thread on the general list so read that for more info. > > http://www.postgresql.org/message-id/520A6E55.40901@cchtml.com > > I'm also going to submit the patch to CommitFest. > +#ifndef WIN32 > if (SOCK_ERRNO == EWOULDBLOCK) > +#else > + if (SOCK_ERRNO == WSAEWOULDBLOCK) > +#endif Thanks for looking into this. I suspect this patch is achieving the right runtime behavior, but some cleanup is in order. src/include/port/win32.h makes some effort to preempt the need for a patch like this, but the relevant code isn't used for MinGW: /** For Microsoft Visual Studio 2010 and above we intentionally redefine* the regular Berkeley error constants and set themto the WSA constants.* Note that this will break if those constants are used for anything else* than Windows Socketserrors.*/ #if _MSC_VER >= 1600 #pragma warning(disable:4005) #define EMSGSIZE WSAEMSGSIZE #define EAFNOSUPPORT WSAEAFNOSUPPORT #define EWOULDBLOCK WSAEWOULDBLOCK #define EPROTONOSUPPORT WSAEPROTONOSUPPORT #define ECONNRESET WSAECONNRESET #define EINPROGRESS WSAEINPROGRESS #define ENOBUFS WSAENOBUFS #define ECONNREFUSED WSAECONNREFUSED #define EOPNOTSUPP WSAEOPNOTSUPP #pragma warning(default:4005) #endif I suspect we should do one of the following: 1. Redefine those constants for more (all?) compilers. 2. Remove that block and put #ifdef around all usage of such constants in frontend code, as you have done. 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, so frontend code can treat errno like backend code treats errno. What do you recommend? Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 08/17/2013 01:16 AM, Noah Misch wrote: > On Fri, Aug 16, 2013 at 06:56:45PM -0500, Michael Cronenworth wrote: >> I started a thread on the general list so read that for more info. >> >> http://www.postgresql.org/message-id/520A6E55.40901@cchtml.com >> >> I'm also going to submit the patch to CommitFest. >> +#ifndef WIN32 >> if (SOCK_ERRNO == EWOULDBLOCK) >> +#else >> + if (SOCK_ERRNO == WSAEWOULDBLOCK) >> +#endif > Thanks for looking into this. I suspect this patch is achieving the right > runtime behavior, but some cleanup is in order. src/include/port/win32.h > makes some effort to preempt the need for a patch like this, but the relevant > code isn't used for MinGW: > > /* > * For Microsoft Visual Studio 2010 and above we intentionally redefine > * the regular Berkeley error constants and set them to the WSA constants. > * Note that this will break if those constants are used for anything else > * than Windows Sockets errors. > */ > #if _MSC_VER >= 1600 > #pragma warning(disable:4005) > #define EMSGSIZE WSAEMSGSIZE > #define EAFNOSUPPORT WSAEAFNOSUPPORT > #define EWOULDBLOCK WSAEWOULDBLOCK > #define EPROTONOSUPPORT WSAEPROTONOSUPPORT > #define ECONNRESET WSAECONNRESET > #define EINPROGRESS WSAEINPROGRESS > #define ENOBUFS WSAENOBUFS > #define ECONNREFUSED WSAECONNREFUSED > #define EOPNOTSUPP WSAEOPNOTSUPP > #pragma warning(default:4005) > #endif > > I suspect we should do one of the following: > > 1. Redefine those constants for more (all?) compilers. > 2. Remove that block and put #ifdef around all usage of such constants in > frontend code, as you have done. > 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, > so frontend code can treat errno like backend code treats errno. > > What do you recommend? > We don't seem to have a problem with this on native builds, only on cross-compiles AFAIK (see buildfarm for proof). The native mingw-w64 build works just fine. So my first question is going to be why is the cross-compile different? cheers andrew
On Sat, Aug 17, 2013 at 02:04:24PM -0400, Andrew Dunstan wrote: >> On Fri, Aug 16, 2013 at 06:56:45PM -0500, Michael Cronenworth wrote: >>> +#ifndef WIN32 >>> if (SOCK_ERRNO == EWOULDBLOCK) >>> +#else >>> + if (SOCK_ERRNO == WSAEWOULDBLOCK) >>> +#endif > We don't seem to have a problem with this on native builds, only on > cross-compiles AFAIK (see buildfarm for proof). The native mingw-w64 > build works just fine. So my first question is going to be why is the > cross-compile different? One of the reports Michael cited was apparently "native": http://www.postgresql.org/message-id/E1UcLPd-0000L4-TI@wrigleys.postgresql.org Perhaps only some versions of w32api trigger the problem. I agree we ought to understand the necessary conditions before proceeding. -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 08/17/2013 01:16 AM, Noah Misch wrote: > On Fri, Aug 16, 2013 at 06:56:45PM -0500, Michael Cronenworth wrote: >> I started a thread on the general list so read that for more info. >> >> http://www.postgresql.org/message-id/520A6E55.40901@cchtml.com >> >> I'm also going to submit the patch to CommitFest. >> +#ifndef WIN32 >> if (SOCK_ERRNO == EWOULDBLOCK) >> +#else >> + if (SOCK_ERRNO == WSAEWOULDBLOCK) >> +#endif > Thanks for looking into this. I suspect this patch is achieving the right > runtime behavior, but some cleanup is in order. src/include/port/win32.h > makes some effort to preempt the need for a patch like this, but the relevant > code isn't used for MinGW: > > /* > * For Microsoft Visual Studio 2010 and above we intentionally redefine > * the regular Berkeley error constants and set them to the WSA constants. > * Note that this will break if those constants are used for anything else > * than Windows Sockets errors. > */ > #if _MSC_VER >= 1600 > #pragma warning(disable:4005) > #define EMSGSIZE WSAEMSGSIZE > #define EAFNOSUPPORT WSAEAFNOSUPPORT > #define EWOULDBLOCK WSAEWOULDBLOCK > #define EPROTONOSUPPORT WSAEPROTONOSUPPORT > #define ECONNRESET WSAECONNRESET > #define EINPROGRESS WSAEINPROGRESS > #define ENOBUFS WSAENOBUFS > #define ECONNREFUSED WSAECONNREFUSED > #define EOPNOTSUPP WSAEOPNOTSUPP > #pragma warning(default:4005) > #endif > > I suspect we should do one of the following: > > 1. Redefine those constants for more (all?) compilers. > 2. Remove that block and put #ifdef around all usage of such constants in > frontend code, as you have done. > 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, > so frontend code can treat errno like backend code treats errno. > > What do you recommend? > > Thanks, > nm > There is a much simpler fix, which is to do these assignments unconditionally in src/port/win32.h. The following small change fixes the problem for me: diff --git a/src/include/port/win32.h b/src/include/port/win32.h index 3a68ea4..5b93220 100644 --- a/src/include/port/win32.h +++ b/src/include/port/win32.h @@ -278,9 +278,8 @@ typedef int pid_t; #ifndef EAFNOSUPPORT #define EAFNOSUPPORT WSAEAFNOSUPPORT #endif -#ifndef EWOULDBLOCK +#undef EWOULDBLOCK #define EWOULDBLOCK WSAEWOULDBLOCK -#endif #ifndef ECONNRESET #define ECONNRESET WSAECONNRESET #endif Note that the original patch appears to be not only misguided but wrong, in that it undid a recent important change (commit a099482c) as I read it. cheers andrew
On 08/17/2013 12:16 AM, Noah Misch wrote: > 1. Redefine those constants for more (all?) compilers. > 2. Remove that block and put #ifdef around all usage of such constants in > frontend code, as you have done. > 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, > so frontend code can treat errno like backend code treats errno. > > What do you recommend? Option 1 is dangerous. I'd rather let the environments keep their constants. Option 2 is the least dangerous but it adds lines of code. Option 3: The errno variable is not set in Windows so relying on it is not possible. If no one likes my patch then you need to come up with your own constants (ex. PG_EINPROGRESS) and define those based on the compiler environment.
On 08/18/2013 12:02 PM, Andrew Dunstan wrote: > There is a much simpler fix, which is to do these assignments unconditionally in > src/port/win32.h. The following small change fixes the problem for me: > No. Please do not do this. > > Note that the original patch appears to be not only misguided but wrong, in that > it undid a recent important change (commit a099482c) as I read it. My patch was created against the latest git checkout as of the date I sent the e-mail. If you could provide the full commit ID I could take a look, but it seems you don't care about my patch enough I'm not sure you will bother.
On 08/17/2013 02:33 PM, Noah Misch wrote: > One of the reports Michael cited was apparently "native": > http://www.postgresql.org/message-id/E1UcLPd-0000L4-TI@wrigleys.postgresql.org > > Perhaps only some versions of w32api trigger the problem. I agree we ought to > understand the necessary conditions before proceeding. Correct. This is *not* a gcc/cross-compile issue. It's a conditional variable definition issue.
On 08/19/2013 09:50 AM, Michael Cronenworth wrote: > On 08/18/2013 12:02 PM, Andrew Dunstan wrote: >> There is a much simpler fix, which is to do these assignments unconditionally in >> src/port/win32.h. The following small change fixes the problem for me: >> > No. Please do not do this. If you object to a proposal then you need to explain what's wrong with it. Note that we already do exactly what is proposed here for EAGAIN and EINTR. In fact there is probably a good argument for doing it for all these constants. I tested the patch I made. The environment was a linux hosted cross-compile, with the latest mingw-64 compiler. Before the patch the problem complained of was exhibited. After the patch it was not. > >> Note that the original patch appears to be not only misguided but wrong, in that >> it undid a recent important change (commit a099482c) as I read it. > My patch was created against the latest git checkout as of the date I sent the > e-mail. If you could provide the full commit ID I could take a look, but it > seems you don't care about my patch enough I'm not sure you will bother. > > I already gave you a sufficient identifier for the commit. In case you're not aware, git is quite happy dealing with small commit identifiers. If you do "git log -1 a099482" you should get the details you require. Frankly, we are not going to go through the code littering it with WSA constants inside #ifdef's without a very good reason. cheers andrew
On 08/19/2013 09:11 AM, Andrew Dunstan wrote: > I already gave you a sufficient identifier for the commit. In case you're not > aware, git is quite happy dealing with small commit identifiers. If you do "git > log -1 a099482" you should get the details you require. It should cross your mind that people have more than one computer and may reply using a mobile phone that doesn't have git loaded on it. When I'm back at my Postgres development system I'll gladly look at the git history. P.S. The tone of your replies is very hostile and unwelcome.
On Mon, Aug 19, 2013 at 10:18 AM, Michael Cronenworth <mike@cchtml.com> wrote: > On 08/19/2013 09:11 AM, Andrew Dunstan wrote: >> I already gave you a sufficient identifier for the commit. In case you're not >> aware, git is quite happy dealing with small commit identifiers. If you do "git >> log -1 a099482" you should get the details you require. > > It should cross your mind that people have more than one computer and may reply > using a mobile phone that doesn't have git loaded on it. > > When I'm back at my Postgres development system I'll gladly look at the git history. > > P.S. The tone of your replies is very hostile and unwelcome. Uh... I don't think Andrew has said anything terribly hostile. He claims that your patch reverts a change made by another recent patch, and he's right. He doesn't mean that you intentionally did that by basing your patch off the wrong git commit; he means that the changes in your patch happen to be the opposite of part of what was in that previous patch. That means that either the previous patch was wrong, or yours is. I'm not sure why it should be Andrew's job to care about which machines you have git installed on. If you can't look up the SHA he provided on the platform you're currently using, that's your problem, not his. This is not exactly time-critical, so you could have just as easily waited until you were back at your laptop before replying. TBH, I think Andrew is going above and beyond the call of duty by developing and testing a patch for the problem you reported. And FWIW, I also agree that we should avoid putting in direct references to WSA* constants in various portions of the code base. We've worked pretty hard to paper over to the differences between Windows and everything else, and the reward of that labor is that 95% of the code we write doesn't have to care about which operating system it's running on. That's a good thing, because many developers - such as myself - do not have Windows development environments set up, and even those that do will not welcome an increased need to test a proposed patch on multiple platforms. I think everyone here accepts that some level of additional effort is going to be needed, on an ongoing basis, to support Windows. But keeping that to a minimum is important. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Michael Cronenworth wrote: > On 08/17/2013 12:16 AM, Noah Misch wrote: > > 1. Redefine those constants for more (all?) compilers. > > 2. Remove that block and put #ifdef around all usage of such constants in > > frontend code, as you have done. > > 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, > > so frontend code can treat errno like backend code treats errno. > > > > What do you recommend? > > Option 1 is dangerous. How so? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 08/19/2013 10:35 AM, Robert Haas wrote: > TBH, I think Andrew is going above and beyond the call of duty by > developing and testing a patch for the problem you reported. I've actually known about this problem for a while, but mistakenly thought it only occurred for cross-compilation: <http://people.planetpostgresql.org/andrew/index.php?/archives/264-Cross-compiling-PostgreSQL-for-WIndows.html>, so I'm at least grateful for this thread having prodded me about it, as well as pointing fairly closely to the problem area. cheers andrew
On Sun, Aug 18, 2013 at 01:02:57PM -0400, Andrew Dunstan wrote: > On 08/17/2013 01:16 AM, Noah Misch wrote: >> /* >> * For Microsoft Visual Studio 2010 and above we intentionally redefine >> * the regular Berkeley error constants and set them to the WSA constants. >> * Note that this will break if those constants are used for anything else >> * than Windows Sockets errors. >> */ >> #if _MSC_VER >= 1600 >> #pragma warning(disable:4005) >> #define EMSGSIZE WSAEMSGSIZE >> #define EAFNOSUPPORT WSAEAFNOSUPPORT >> #define EWOULDBLOCK WSAEWOULDBLOCK >> #define EPROTONOSUPPORT WSAEPROTONOSUPPORT >> #define ECONNRESET WSAECONNRESET >> #define EINPROGRESS WSAEINPROGRESS >> #define ENOBUFS WSAENOBUFS >> #define ECONNREFUSED WSAECONNREFUSED >> #define EOPNOTSUPP WSAEOPNOTSUPP >> #pragma warning(default:4005) >> #endif >> >> I suspect we should do one of the following: >> >> 1. Redefine those constants for more (all?) compilers. >> 2. Remove that block and put #ifdef around all usage of such constants in >> frontend code, as you have done. >> 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, >> so frontend code can treat errno like backend code treats errno. > There is a much simpler fix, which is to do these assignments > unconditionally in src/port/win32.h. The following small change fixes > the problem for me: That was option #1. (You weren't planning to change just the one symbol causing the failure at hand, were you?) Reasonable choice. The point in the code comment quoted above looks bad, but the lack of reports of that nature against official 9.2 binaries corroborates it having not been a problem yet. The only non-socket use I see for the constants in question is the EINTR test in XLogWrite(), which probably can't happen on Windows. > Note that the original patch appears to be not only misguided but wrong, > in that it undid a recent important change (commit a099482c) as I read > it. Ah; true enough. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 08/19/2013 07:35 PM, Noah Misch wrote: > That was option #1. (You weren't planning to change just the one symbol > causing the failure at hand, were you?) Reasonable choice. The point in the > code comment quoted above looks bad, but the lack of reports of that nature > against official 9.2 binaries corroborates it having not been a problem yet. > The only non-socket use I see for the constants in question is the EINTR test > in XLogWrite(), which probably can't happen on Windows. Redefining compiler constants is bad bandaid. A similar bandaid was in place to begin with caused this problem. If you believe PostgreSQL will never use code that needs the true errno value then go ahead with Option 1. > Ah; true enough. After looking over the changes it was a merge mistake. I originally wrote the patch on the 9.2 branch and ported it to master. Nothing to call "misguided". Just so that everyone understands: I was looking for comments to discuss a solution to the socket error checking problem. I'm not looking to shove anything down one's throat.
On 08/19/2013 11:36 PM, Michael Cronenworth wrote: > On 08/19/2013 07:35 PM, Noah Misch wrote: >> That was option #1. (You weren't planning to change just the one symbol >> causing the failure at hand, were you?) Reasonable choice. The >> point in the >> code comment quoted above looks bad, but the lack of reports of that >> nature >> against official 9.2 binaries corroborates it having not been a >> problem yet. >> The only non-socket use I see for the constants in question is the >> EINTR test >> in XLogWrite(), which probably can't happen on Windows. > > Redefining compiler constants is bad bandaid. A similar bandaid was in > place to begin with caused this problem. If you believe PostgreSQL > will never use code that needs the true errno value then go ahead with > Option 1. No the reverse is the case. The real problem is that the bandaid was not applied sufficiently widely. What we propose is exactly what is already in use for the Microsoft compilers, and has thus been well and truly tested over some years. Changing these definitions doesn't change the value of errno in the slightest - it only changes the values that we test for. The caveat in the MSVC-specific code that Noah pointed to still applies, but it appears that we don't in fact use these constants anywhere other than in relation to sockets. I'm about to update my buildfarm member jacana so it has the latest mingw-w64 compiler and this should exhibit this error. Then I'll apply a patch along the lines of option 1. If nothing breaks, I'll backpatch that to 9.1 where we enabled use of this compiler. cheers andrew
On Mon, Aug 19, 2013 at 08:46:11AM -0500, Michael Cronenworth wrote: > On 08/17/2013 12:16 AM, Noah Misch wrote: > > 1. Redefine those constants for more (all?) compilers. > > 2. Remove that block and put #ifdef around all usage of such constants in > > frontend code, as you have done. > > 3. Remove that block and make src/backend/port/win32/socket.c frontend-usable, > > so frontend code can treat errno like backend code treats errno. > > > > What do you recommend? > > Option 1 is dangerous. I'd rather let the environments keep their constants. I concur, but our field experience doing it this way lessens my concern. > Option 2 is the least dangerous but it adds lines of code. More than that, as Robert explained, we seek to _isolate_ Windows-specific code. That's true even when a better-isolated approach takes more lines. > Option 3: The errno variable is not set in Windows so relying on it is not possible. Look at src/backend/port/win32/socket.c; it emulates POSIX socket interfaces on Windows, and that emulation includes setting errno. I'd favor option 3 if we weren't already successfully using option 1 for Visual Studio 2010, the compiler of official 9.2 and 9.3 binaries. Since we have been doing that, I figure changing to option 3 now is riskier than staying the course. nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
On 08/21/2013 10:06 PM, Noah Misch wrote: > I concur, but our field experience doing it this way lessens my concern. I see this change has hit master. I've pulled in the new patch for the Fedora MinGW package. Thanks, Michael