An extra error for client disconnection on Windows
Hello.
After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.
LOG: could not receive data from client: An existing connection was forcibly closed by the remote host.
This is because pgwin32_recv reuturns an error ECONNRESET for the
situation instead of returning non-error EOF as recv(2) does.
This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.
The attached patch does this.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
0001-Avoid-unnecessary-error-message-on-Windows.patchtext/x-patch; charset=us-asciiDownload
From 7106c56c6606af25ce65b0f5ece8dae095e7c756 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 2 Jun 2016 09:53:56 +0900
Subject: [PATCH] Avoid unnecessary error message on Windows
On a client process termination on Windows, server emits an error
message which is not seen on Linux boxes. This is caused by a
difference in handling the situation by recv(). This patch translates
WSACONNRESET of WSARecv to just an EOF so that the pgwin32_recv()
behaves as the same with recv() on Linux.
---
src/backend/port/win32/socket.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c
index 5d8fb7f..9d4ac6d 100644
--- a/src/backend/port/win32/socket.c
+++ b/src/backend/port/win32/socket.c
@@ -372,6 +372,7 @@ pgwin32_recv(SOCKET s, char *buf, int len, int f)
DWORD b;
DWORD flags = f;
int n;
+ int lasterror;
if (pgwin32_poll_signals())
return -1;
@@ -383,7 +384,13 @@ pgwin32_recv(SOCKET s, char *buf, int len, int f)
if (r != SOCKET_ERROR)
return b; /* success */
- if (WSAGetLastError() != WSAEWOULDBLOCK)
+ lasterror = WSAGetLastError();
+
+ /* Unix's recv treats a connection reset by peer as just an EOF */
+ if (lasterror == WSAECONNRESET)
+ return 0;
+
+ if (lasterror != WSAEWOULDBLOCK)
{
TranslateSocketError();
return -1;
@@ -410,7 +417,14 @@ pgwin32_recv(SOCKET s, char *buf, int len, int f)
r = WSARecv(s, &wbuf, 1, &b, &flags, NULL, NULL);
if (r != SOCKET_ERROR)
return b; /* success */
- if (WSAGetLastError() != WSAEWOULDBLOCK)
+
+ lasterror = WSAGetLastError();
+
+ /* Unix's recv treats a connection reset by peer as just an EOF */
+ if (lasterror == WSAECONNRESET)
+ return 0;
+
+ if (lasterror != WSAEWOULDBLOCK)
{
TranslateSocketError();
return -1;
--
1.8.3.1
On Thu, Jun 2, 2016 at 4:51 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.LOG: could not receive data from client: An existing connection was forcibly closed by the remote host.
This is because pgwin32_recv reuturns an error ECONNRESET for the
situation instead of returning non-error EOF as recv(2) does.This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.The attached patch does this.
Please add this to the next CommitFest so it gets reviewed.
--
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
Thank you for the suggestion, I've done it.
At Wed, 15 Jun 2016 12:15:07 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmoadQzVLG3ONw=FCGOcQxxDwP7_9AGQD43S7Z+hQj56WYg@mail.gmail.com>
On Thu, Jun 2, 2016 at 4:51 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.LOG: could not receive data from client: An existing connection was forcibly closed by the remote host.
This is because pgwin32_recv reuturns an error ECONNRESET for the
situation instead of returning non-error EOF as recv(2) does.This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.The attached patch does this.
Please add this to the next CommitFest so it gets reviewed.
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:
Hello.
After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.LOG: could not receive data from client: An existing connection was
forcibly closed by the remote host.
This is because pgwin32_recv reuturns an error ECONNRESET for the
situation instead of returning non-error EOF as recv(2) does.This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.The attached patch does this.
I reviewed and verified the changes. This patch works as it stats.
Now there is no extra error message that occurs whenever a client
disconnects abnormally.
Marked the patch as "ready for committer".
Regards,
Hari Babu
Fujitsu Australia
Haribabu Kommi <kommi.haribabu@gmail.com> writes:
On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.
LOG: could not receive data from client: An existing connection was
forcibly closed by the remote host.This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.
Marked the patch as "ready for committer".
Windows is not my platform, but ... is this actually an improvement?
I'm fairly concerned that this change would mask real errors that ought
to get logged. I don't know that that's an okay price to pay for
suppressing a log message when clients violate the protocol.
According to
https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
WSAECONNRESET means:
An existing connection was forcibly closed by the remote host. This
normally results if the peer application on the remote host is
suddenly stopped, the host is rebooted, the host or remote network
interface is disabled, or the remote host uses a hard close (see
setsockopt for more information on the SO_LINGER option on the remote
socket). This error may also result if a connection was broken due to
keep-alive activity detecting a failure while one or more operations
are in progress. Operations that were in progress fail with
WSAENETRESET. Subsequent operations fail with WSAECONNRESET.
(The description of WSAENETRESET, on the same page, indicates that the
last two sentences apply only to the keep-alive failure case.)
So this change would deal nicely with the "peer application on the remote
host is suddenly stopped" case, at the price of being not nice about any
of the other cases. Not convinced it's a good tradeoff.
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
On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Haribabu Kommi <kommi.haribabu@gmail.com> writes:
On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyotaro@lab.ntt.co.jp> wrote:After a process termination without PQfinish() of a client,
server emits the following log message not seen on Linux boxes.
LOG: could not receive data from client: An existing connection was
forcibly closed by the remote host.This patch translates WSAECONNRESET of WSARecv to an EOF so that
pgwin32_recv behaves the same way with Linux.Marked the patch as "ready for committer".
Windows is not my platform, but ... is this actually an improvement?
I'm fairly concerned that this change would mask real errors that ought
to get logged. I don't know that that's an okay price to pay for
suppressing a log message when clients violate the protocol.According to
https://msdn.microsoft.com/en-us/library/windows/desktop/ms740668(v=vs.85).aspx
WSAECONNRESET means:An existing connection was forcibly closed by the remote host. This
normally results if the peer application on the remote host is
suddenly stopped, the host is rebooted, the host or remote network
interface is disabled, or the remote host uses a hard close (see
setsockopt for more information on the SO_LINGER option on the remote
socket). This error may also result if a connection was broken due to
keep-alive activity detecting a failure while one or more operations
are in progress. Operations that were in progress fail with
WSAENETRESET. Subsequent operations fail with WSAECONNRESET.(The description of WSAENETRESET, on the same page, indicates that the
last two sentences apply only to the keep-alive failure case.)So this change would deal nicely with the "peer application on the remote
host is suddenly stopped" case, at the price of being not nice about any
of the other cases. Not convinced it's a good tradeoff.
Yes, in the list of failure cases that could trigger this error, the
one that looks like a problem is to me is when a network interface is
disabled. It may be a good idea to let users know via the logs that
something was connected. Could we for example log a WARNING message,
and not report an error?
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So this change would deal nicely with the "peer application on the remote
host is suddenly stopped" case, at the price of being not nice about any
of the other cases. Not convinced it's a good tradeoff.
Yes, in the list of failure cases that could trigger this error, the
one that looks like a problem is to me is when a network interface is
disabled. It may be a good idea to let users know via the logs that
something was connected. Could we for example log a WARNING message,
and not report an error?
It isn't an "error". These conditions get logged at COMMERROR which is
effectively LOG_SERVER_ONLY.
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
Hello, thanks for revewing and the discussion.
At Sat, 10 Sep 2016 10:44:50 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <17326.1473518690@sss.pgh.pa.us>
Michael Paquier <michael.paquier@gmail.com> writes:
On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So this change would deal nicely with the "peer application on the remote
host is suddenly stopped" case, at the price of being not nice about any
of the other cases. Not convinced it's a good tradeoff.Yes, in the list of failure cases that could trigger this error, the
one that looks like a problem is to me is when a network interface is
disabled. It may be a good idea to let users know via the logs that
something was connected. Could we for example log a WARNING message,
and not report an error?
This "error" won't be raised when any side of NIC stopped. This
error is returned when the connection was "resetted", that is,
RST packet is sent and received from the peer. "connection reset"
is regarded as just a EOF at least for read() on Linux, I don't
think there's no problem to conceal the ECONNRESET on Windows if
we are satisfied with the behavior of Linux's read(). Users won't
know of the closed connections if a client doesn't show a message
for an EOF on Linux. Users will know of them on Windows if a
program shows a message for an EOF without a message for
ECONNRESET.
In another aspect is the policy of behavior unification.
If we take a policy to try to imitate the behavior of some
reference platform (specifically Linux) on other platforms, this
is required disguising. Another potential policy on this problem
is "following the platform's behavior". From this viewpoint, this
message should be shown to users because Windows says
so. Especially for socket operations, the simultion layer is
intending the former for non-error behaviors, but I'm not sure
about the behaviors on errors.
It isn't an "error". These conditions get logged at COMMERROR which is
effectively LOG_SERVER_ONLY.
If RST is not expected at the time and distinguishing it from FIN
is significant to users, it would be an "error".
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
If we take a policy to try to imitate the behavior of some
reference platform (specifically Linux) on other platforms, this
is required disguising. Another potential policy on this problem
is "following the platform's behavior". From this viewpoint, this
message should be shown to users because Windows says
so. Especially for socket operations, the simultion layer is
intending the former for non-error behaviors, but I'm not sure
about the behaviors on errors.
The more you hack windows, the more you'll notice that it is full of
caveats, behavior exceptions, and that it runs in its way as nothing
else in this world... This patch looks like a tempest in a teapot at
the end. Why is it actually a problem to show this message? Just
useless noise? If that's the only reason let's drop the patch and move
on. It seems that the extra information that could be fetched
depending on what caused the connection reset is not worth the risk.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier wrote:
On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:If we take a policy to try to imitate the behavior of some
reference platform (specifically Linux) on other platforms, this
is required disguising. Another potential policy on this problem
is "following the platform's behavior". From this viewpoint, this
message should be shown to users because Windows says
so. Especially for socket operations, the simultion layer is
intending the former for non-error behaviors, but I'm not sure
about the behaviors on errors.The more you hack windows, the more you'll notice that it is full of
caveats, behavior exceptions, and that it runs in its way as nothing
else in this world... This patch looks like a tempest in a teapot at
the end. Why is it actually a problem to show this message? Just
useless noise? If that's the only reason let's drop the patch and move
on.
Yeah, I looked into this a few days ago and that was my conclusion also:
let's drop this.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 13, 2016 at 10:00 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
Yeah, I looked into this a few days ago and that was my conclusion also:
let's drop this.
Okay. Just done so in the CF app.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
At Tue, 13 Sep 2016 10:00:32 -0300, Alvaro Herrera <alvherre@2ndquadrant.com> wrote in <20160913130032.GA391646@alvherre.pgsql>
Michael Paquier wrote:
On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:If we take a policy to try to imitate the behavior of some
reference platform (specifically Linux) on other platforms, this
is required disguising. Another potential policy on this problem
is "following the platform's behavior". From this viewpoint, this
message should be shown to users because Windows says
so. Especially for socket operations, the simultion layer is
intending the former for non-error behaviors, but I'm not sure
about the behaviors on errors.The more you hack windows, the more you'll notice that it is full of
caveats, behavior exceptions, and that it runs in its way as nothing
else in this world... This patch looks like a tempest in a teapot at
the end. Why is it actually a problem to show this message? Just
useless noise? If that's the only reason let's drop the patch and move
on.Yeah, I looked into this a few days ago and that was my conclusion also:
let's drop this.
Ok, I greed. Thanks.
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers