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

On Mon, Mar 7, 2011 at 6:21 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> 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.

Yeah, that's bad. At the risk of repeating myself, I don't think this
code should be checking SyncRepRequested() in the first place. If the
user has turned off synchronous_commit, then we should just commit
asynchronously, even if sync rep is otherwise in force. Otherwise,
this if statement is going to get really complicated. The logic is
already at least mildly wrong here anyway: clearly we do NOT need to
commit synchronously if the transaction has not written xlog, even if
sync rep is enabled.

> -                       /* 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.

I could go either way on this but clearly we need to do one or the other.

> +       /*
> +        * 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.

Fixed.

> I found some typos. The attached patch fixes them.

Committed with minor changes.

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