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