Re: [COMMITTERS] pgsql: Efficient transaction-controlled synchronous replication.

On Mon, Mar 7, 2011 at 5:27 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Mar 7, 2011 at 7:51 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Efficient transaction-controlled synchronous replication.
>> If a standby is broadcasting reply messages and we have named
>> one or more standbys in synchronous_standby_names then allow
>> users who set synchronous_replication to wait for commit, which
>> then provides strict data integrity guarantees. Design avoids
>> sending and receiving transaction state information so minimises
>> bookkeeping overheads. We synchronize with the highest priority
>> standby that is connected and ready to synchronize. Other standbys
>> can be defined to takeover in case of standby failure.
>>
>> This version has very strict behaviour; more relaxed options
>> may be added at a later date.
>
> Pretty cool! I'd appreciate very much your efforts and contributions.

Here are another comments:

if ((wrote_xlog && XactSyncCommit) || forceSyncCommit || nrels > 0 ||
SyncRepRequested())

Whenever synchronous_replication is TRUE, we disable synchronous_commit.
But, before disabling that, we should check also max_wal_senders and
synchronous_standby_names? Otherwise, synchronous_commit can
be disabled unexpectedly even in non replication case.

- /* Let the master know that we received some data. */
- XLogWalRcvSendReply();
- XLogWalRcvSendHSFeedback();

This change completely eliminates the difference between write_location
and flush_location in pg_stat_replication. If this change is reasoable, we
should get rid of write_location from pg_stat_replication since it's useless.
If not, this change should be reverted. I'm not sure whether monitoring
the difference between write and flush locations is useful. But I guess that
someone thought so and that code was added.

+ /*
+ * Current location of the head of the queue. All waiters should have
+ * a waitLSN that follows this value, or they are currently being woken
+ * to remove themselves from the queue. Protected by SyncRepLock.
+ */
+ XLogRecPtr lsn;

The comment ", or they are currently being woken to remove themselves
from the queue" is no longer required because the proc is currently removed
by walsender.

I found some typos. The attached patch fixes them.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center