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

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

--
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(at)cchtml(dot)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