Re: Expansion of our checks for connection-loss errors

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
Cc: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Expansion of our checks for connection-loss errors
Date: 2020-10-09 16:14:17
Message-ID: 2768918.1602260057@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Hmm, excellent point. While our code response to all these errors
> should be the same, you are right that that doesn't extend to emitting
> identical error texts. For EHOSTUNREACH/ENETDOWN/ENETUNREACH, we
> should say something like "connection to server lost", without claiming
> that the server crashed. It is less clear what to do with ECONNABORTED,
> but I'm inclined to put it in the network-problem bucket not the
> server-crash bucket, despite lorikeet's behavior. Thoughts?

> This also destroys the patch's idea that switch statements should be
> able to handle these all alike. If we group things as "ECONNRESET means
> server crash and the others are all network failures", then I'd be
> inclined to leave the ECONNRESET cases alone and just introduce
> new infrastructure to recognize all the network-failure errnos.

Actually, treating it that way seems like a good thing because it nets
out as (nearly) no change to our error message behavior. The connection
failure errnos fall through to the default case, which produces a
perfectly reasonable report that includes strerror(). The only big thing
we're changing is the set of errnos that errcode_for_socket_access will
map to ERRCODE_CONNECTION_FAILURE, so this is spiritually closer to your
original patch.

Some other changes in the attached v2:

* I incorporated Kyotaro-san's suggested improvements.

* I went ahead and included ENETRESET and EHOSTDOWN, figuring that
if they exist we definitely want to class them as network failures.
We can worry about ifdef'ing them when and if we find a platform
that hasn't got them. (I don't see any non-ugly way to make the
ALL_NETWORK_FAILURE_ERRNOS macro vary for missing symbols, so I'd
rather not deal with that unless it's proven necessary.)

* I noticed that we were not terribly consistent about whether
EPIPE is regarded as indicating a server failure like ECONNRESET
does. So this patch also makes sure that EPIPE is treated like
ECONNRESET everywhere. (Hence, pqsecure_raw_read's error reporting
does change, since it'll now report EPIPE as server failure.)

I lack a way to test this on Windows, but otherwise it feels
like it's about ready.

regards, tom lane

Attachment Content-Type Size
recognize-more-connection-loss-errnos-2.patch text/x-diff 7.2 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-10-09 16:50:36 Re: [PATCH] ecpg: fix progname memory leak
Previous Message Magnus Hagander 2020-10-09 15:48:40 Re: [PATCH] ecpg: fix progname memory leak