On Fri, Feb 18, 2011 at 7:06 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> The patch is very lite touch on a few areas of code, plus a chunk of
> specific code, all on master-side. Pretty straight really. I'm sure
> problems will be found, its not long since I completed this; thanks to
> Daniel Farina for your help with patch assembly.
This looks like it's in far better shape than the previous version.
Thanks to you and Dan for working on it.
As you have this coded, enabling synchronous_replication forces all
transactions to commit synchronously. This means that, when
synchronous_replication=on, you get synchronous_commit=on behavior
even if you have synchronous_commit=off. I'd prefer to see us go the
other way, and say that you can only get synchronous replication when
you're also getting synchronous commit.
Even if not, we should at least arrange the test so that we don't
force synchronous commit for transactions that haven't written XLOG.
That is, instead of:
((wrote_xlog && XactSyncCommit) || forceSyncCommit || nrels > 0 ||
SyncRepRequested())
...we should instead say:
((write_xlog && (XactSyncCommit || SyncRepRequested()) ||
forceSyncCommit || nrels > 0)
...but as I say I'd be inclined to instead keep the existing test, and
just skip SyncRepWaitForLSN() if we aren't committing synchronously.
I'm unsure of the value of sync_replication_timeout_client (which
incidentally is spelled replication_timeout_client in one place, and
elsewhere asynchornus -> asynchronous). I think in practice if you
start hitting that timeout, your server will slow to such a crawl that
you might as well be down. On the other hand, I see no particular
harm in leaving the option in there either, though I definitely think
the default should be changed to -1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company