Re: TCP keepalive support for libpq

On Mon, Feb 15, 2010 at 8:58 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Tue, Feb 16, 2010 at 1:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> I'm all for this as a 9.1 submission, but let's not commit to trying to
>>> debug it now.  I would like a green buildfarm for awhile before we wrap
>>> alpha4, and this sort of untested "it can't hurt" patch is exactly what
>>> is likely to make things not green.
>>
>> Mmm.  OK, fair enough.
>
> Okay. I added the patch to the first CF for v9.1.
> Let's discuss about it later.

There is talk of applying this patch, or something like it, for 9.0,
so I guess now is the time for discussion.  The overriding issue is
that we need walreceiver to notice if the master goes away.  Rereading
this thread, the major argument against applying this patch is that it
changes the default behavior: it ALWAYS enables keepalives, and then
additionally provides libpq parameters to change some related
parameters (number of seconds between sending keepalives, number of
seconds after which to retransmit a keepalive, number of lost
keepalives after which a connection is declared dead).  Although the
consensus seems to be that keepalives are a good idea much more often
than not, I am wary of unconditionally turning on a behavior that has,
in previous releases, been unconditionally turned off.  I don't want
to do this in 9.0, and I don't think I want to do it in 9.1, either.

What I think would make sense is to add an option to control whether
keepalives get turned on.   If you say keepalives=1, you get on = 1;
setsockopt(conn->sock, SOL_SOCKET, SO_KEEPALIVE,
(char *) &on, sizeof(on); if you say keepalives=0, we do nothing
special.  If you say neither, you get the default behavior, which I'm
inclined to make keepalives=1.  That way, everyone gets the benefit of
this patch (keepalives turned on) by default, but if for some reason
someone is using libpq over the deep-space network or a connection for
which they pay by the byte, they can easily shut it off.  We can note
the behavior change under "observe the following incompatibilities".

I am inclined to punt the keepalives_interval, keepalives_idle, and
keepalives_count parameters to 9.1.  If these are needed for
walreciever to work reliably, this whole approach is a dead-end,
because those parameters are not portable.  I will post a patch later
today along these lines.

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