Re: walsender doesn't send keepalives when writes are pending

On 2014-03-05 18:26:13 +0200, Heikki Linnakangas wrote:
> On 02/25/2014 06:41 PM, Robert Haas wrote:
> >On Tue, Feb 25, 2014 at 11:23 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >>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.

I still don't see how my proposed patch increases the dependency, rather
the contrary, it's less dependant on the flushing behaviour.

But I agree that a more sweeping change is a good idea.

> The logic was the same before the patch, but I added the XXX comment above.
> Why do we sleep in increments of 1/10 of wal_sender_timeout? Originally, the
> code calculated when the next wakeup should happen, by adding
> wal_sender_timeout (or replication_timeout, as it was called back then) to
> the time of the last reply. Why don't we do that?
> [ archeology ]

It imo makes sense to wakeup after last_reply + wal_sender_timeout/2, so
a requested reply actually has time to arrive, but otherwise I agree.

I think your patch makes sense. Additionally imo the timeout checking
should be moved outside the if (caughtup || pq_is_send_pending()), but
that's probably a separate patch.

Any chance you could apply your patch soon? I've a patch pending that'll
surely conflict with this and it seems better to fix it first.

Greetings,

Andres Freund