Re: Replication server timeout patch

On Fri, Feb 11, 2011 at 2:02 PM, Daniel Farina <drfarina(at)acm(dot)org> wrote:
> I split this out of the synchronous replication patch for independent
> review. I'm dashing out the door, so I haven't put it on the CF yet or
> anything, but I just wanted to get it out there...I'll be around in
> Not Too Long to finish any other details.

This looks like a useful and separately committable change.

However, it looks to me like this renders wal_sender_delay aka
WalSndDelay completely unused. If we don't need that GUC any more, we
should rip it out completely.

The comment in WalSndHandshake should have a tab at the beginning of
every line. Right now the first line has a tab and the rest have
spaces.

The first hunk in WalSndLoop is a meaningless whitespace change.

I wonder if we ought to just call this replication_timeout, rather
than replication_timeout_server. Simon's patch (from which this
extracted) also has replication_timeout_client, but the two aren't
symmetrical. The replication_timeout_client in this patch is the
amount of time after which the master acknowledges the commit even
though the synchronous standby hasn't acked yet. So it only applies
to the synchronous replication case, whereas this is useful for both
synchronous and asynchronous replication. I'm inclined to think that
knob is utterly useless anyway; surely waiting more than zero will
reduce the throughput of the system to some minute fraction of its
normal value, while waiting less than infinity throws out the data
guarantee that made you pick synchronous replication in the first
place. Even if we do decide to keep that knob, I don't think we'll
want the value to be symmetric with this one.

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