Re: [HACKERS] An extra error for client disconnection on Windows

[Kyotaro HORIGUCHI]

Hello,

At Tue, 13 Sep 2016 10:00:32 -0300, Alvaro Herrera  
wrote in <20160913130032.GA391646@alvherre.pgsql>
> Michael Paquier wrote:
> > On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
> >  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

Re: [HACKERS] An extra error for client disconnection on Windows

[Michael Paquier]

On Tue, Sep 13, 2016 at 10:00 PM, Alvaro Herrera
 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

Re: [HACKERS] An extra error for client disconnection on Windows

[Alvaro Herrera]

Michael Paquier wrote:
> On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
>  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 Herrerahttp://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

Re: [HACKERS] An extra error for client disconnection on Windows

[Michael Paquier]

On Tue, Sep 13, 2016 at 10:42 AM, Kyotaro HORIGUCHI
 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

Re: [HACKERS] An extra error for client disconnection on Windows

[Kyotaro HORIGUCHI]

Hello, thanks for revewing and the discussion.

At Sat, 10 Sep 2016 10:44:50 -0400, Tom Lane  wrote in 
<17326.1473518...@sss.pgh.pa.us>
> Michael Paquier  writes:
> > On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane  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

Re: [HACKERS] An extra error for client disconnection on Windows

[Tom Lane]

Michael Paquier  writes:
> On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane  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

Re: [HACKERS] An extra error for client disconnection on Windows

[Michael Paquier]

On Fri, Sep 9, 2016 at 11:39 PM, Tom Lane  wrote:
> Haribabu Kommi  writes:
>> On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
>> horiguchi.kyot...@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

Re: [HACKERS] An extra error for client disconnection on Windows

[Tom Lane]

Haribabu Kommi  writes:
> On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
> horiguchi.kyot...@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

Re: [HACKERS] An extra error for client disconnection on Windows

[Haribabu Kommi]

On Thu, Jun 2, 2016 at 6:51 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@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

Re: [HACKERS] An extra error for client disconnection on Windows

[Kyotaro HORIGUCHI]

Thank you for the suggestion, I've done it.

At Wed, 15 Jun 2016 12:15:07 -0400, Robert Haas  wrote 
in 
> On Thu, Jun 2, 2016 at 4:51 AM, Kyotaro HORIGUCHI
>  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

Re: [HACKERS] An extra error for client disconnection on Windows

[Robert Haas]

On Thu, Jun 2, 2016 at 4:51 AM, Kyotaro HORIGUCHI
 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

[HACKERS] An extra error for client disconnection on Windows

[Kyotaro HORIGUCHI]

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
>From 7106c56c6606af25ce65b0f5ece8dae095e7c756 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
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, , 1, , , 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers