Обсуждение: 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

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

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

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
theremote host is   suddenly stopped, the host is rebooted, the host or remote network   interface is disabled, or the
remotehost uses a hard close (see   setsockopt for more information on the SO_LINGER option on the remote   socket).
Thiserror 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

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

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

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

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

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

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

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