Re: Expansion of our checks for connection-loss errors

At Thu, 08 Oct 2020 21:41:55 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> writes:
> > At Thu, 08 Oct 2020 15:15:54 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> >> Accordingly, I propose the attached patch (an expansion of
> >> Fujii-san's) that causes us to test for all five errnos anyplace
> >> we had been checking for ECONNRESET.
>
> > +1 for the direction.
>
> > In terms of connection errors, connect(2) and bind(2) can return
> > EADDRNOTAVAIL. bind(2) and listen(2) can return EADDRINUSE. FWIW I
> > recetnly saw pgbench getting EADDRNOTAVAIL. (They have mapping from
> > respective WSA errors in TranslateSocketError())
>
> I do not think we have any issues with connection-time errors;
> or at least, if we do, the spots being touched here certainly
> shouldn't need to worry about them. These places are dealing
> with already-established connections.

errcode_for_socket_access() is called for connect, bind and lesten but
I understand we don't consider the case since we don't have an actual
issue related to the functions.

> > I'd make errno_is_connection_loss use ALL_CONNECTION_LOSS_ERRNOS to
> > avoid duplication definition of the errno list.
>
> Hmm, might be worth doing, but I'm not sure. I am worried about
> whether compilers will generate equally good code that way.

The two are placed side-by-side so either will do for me.

> > - if (ret < 0 && WSAGetLastError() == WSAECONNRESET)
> > + if (ret < 0 && errno_is_connection_loss(WSAGetLastError()))
>
> > Don't we need to use TranslateSocketError() before?
>
> Oh, I missed that. But:
>
> > Perhaps I'm confused but SOCK_ERROR doesn't seem portable between
> > Windows and Linux.
>
> In that case, nothing would have worked on Windows for the last
> ten years, so you're mistaken. I think the actual explanation
> why this works, and why that test in parallel.c probably still
> works even with my mistake, is that win32_port.h makes sure that
> our values of ECONNRESET etc match WSAECONNRESET etc.

Mmmmmmmmmm. Sure.

> IOW, we'd not actually need TranslateSocketError at all, except
> that it maps some not-similarly-named error codes for conditions
> that don't exist in Unix into ones that do. We probably do want
> TranslateSocketError in this parallel.c test so that anything that
> it maps to one of the errno_is_connection_loss codes will be
> recognized; but the basic cases would work anyway, unless I
> misunderstand this stuff entirely.

Yeah, that seems to work.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center