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@2ndquadrant.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