On Tue, Feb 25, 2014 at 11:23 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-02-25 11:15:46 -0500, Robert Haas wrote:
>> On Tue, Feb 25, 2014 at 10:54 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > I am not sure I can follow. Why doesn't it make sense to send out the
>> > keepalive (with replyRequested = true) when we're busy sending stuff
>> > (which will be the case most of the time on a busy server)?
>>
>> It may very well make sense, but your patch won't generally have that
>> effect, because with the patch you proposed, the keep-alive can only
>> be sent when the server is busy if the write queue is also full.
>
> Well, it either needs to be caughtup *or*/and have a busy write queue,
> right?
Right.
> Usually that state will be reached very quickly because before
> that we're writing data to the network as fast as it can be read from
> disk.
I'm unimpressed. Even if that is in practice true, making the code
self-consistent is a goal of non-trivial value. The timing of sending
keep-alives has no business depending on the state of the write queue,
and right now it doesn't. Your patch would make it depend on that,
mostly by accident AFAICS.
> Also, there's no timeout checks outside that if (caughtup ||
> send_pending()) block, so there's not much of a window to hit problems when
> that loop isn't entered.
That might be true, but waiting until the write queue is full to send
a ping and then checking whether we've timed out before the queue has
drained isn't a sane design pattern.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
