Re: TCP keepalive support for libpq

On Tue, Jun 22, 2010 at 15:20, Robert Haas <robertmhaas@gmail.com> wrote:
> 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".

+1 on enabling it by default, but providing a switch to turn it off.


> 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.

Do we know how unportable? If it still helps the majority, it might be
worth doing. But I agree, if it's not really needed for walreceiver,
then it should be punted to 9.1.


-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/