Re: Dangling Client Backend Process

On Fri, Oct 30, 2015 at 11:03 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-10-30 10:57:45 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>> > adding a parseInput(conn) into the loop yields the expected
>> > FATAL:  57P01: terminating connection due to unexpected postmaster exit
>> > Is there really any reason not to do that?
>>
>> Might work, but it probably needs some study:
>
> Yea, definitely. I was just at pgconf.eu's keynote catching up on a
> talk. No fully thought through proposal's to be expected ;)
>
>> (a) is it safe
>
> I don't immediately see why not.
>
>> (b) is this the right place / are there other places
>
> I think it's ok for the send failure case, in a quick lookthrough I
> didn't find anything else for writes - I'm not entirely sure all read
> cases are handled tho, but it seems less likely to be mishandles.

pqHandleSendFailure() has this comment:

 * Primarily, what we want to accomplish here is to process an async
 * NOTICE message that the backend might have sent just before it died.

And also this comment:

     * Accept any available input data, ignoring errors.  Note that if
     * pqReadData decides the backend has closed the channel, it will close
     * our side of the socket --- that's just what we want here.

And finally this comment:

     * Parse any available input messages.  Since we are in PGASYNC_IDLE
     * state, only NOTICE and NOTIFY messages will be eaten.

Now, taken together, these messages suggest two conclusions:

1. The decision to ignore errors here was deliberate.
2. Calling pqParseInput() wouldn't notice errors anyway because the
connection is PGASYNC_IDLE.

With respect to the first conclusion, I think it's fair to argue that,
while this may have seemed like a reasonable idea at the time, it's
probably not what we want any more.  Quite apart from the patch
proposed here, ProcessInterrupts() has assume for years (certainly
since 9.0, I think) that it's legitimate to signal a FATAL error to an
idle client and assume that the client will read that error as a
response to its next protocol message.  So it seems to me that this
function should be adjusted to notice errors, and not just notice and
notify messages.

The second conclusion does not appear to be correct.  parseInput()
will call pqParseInput3() or pqParseInput2(), either of which will
handle an error as if it were a notice - i.e. by printing it out.

Here's a patch based on that analysis, addressing just that one
function, not any of the other changes talked about on this thread.
Does this make sense?  Would we want to back-patch it, and if so how
far, or just adjust master?  My gut is just master, but I don't know
why this issue wouldn't also affect Hot Standby kills and maybe other
kinds of connection termination situations, so maybe there's an
argument for back-patching.  On the third hand, failing to read the
error message off of a just-terminated connection isn't exactly a
crisis of the first order either.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company