Re: Sync Rep v19

On Thu, Mar 3, 2011 at 7:53 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Latest version of Sync Rep, which includes substantial internal changes
> and simplifications from previous version. (25-30 changes).
>
> Includes all outstanding technical comments, typos and docs. I will
> continue to work on self review and test myself, though actively
> encourage others to test and report issues.

Thanks for the patch!

> * synchronous_standby_names = "*" matches all standby names

Using '*' as the default seems to lead the performance degradation by
being connected from unexpected synchronous standby.

> * pg_stat_replication now shows standby priority - this is an ordinal
> number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> standby".

monitoring.sgml should be updated.

Though I've not read whole of the patch yet, here is the current comment:

Using MyProc->lwWaiting and lwWaitLink for backends to wait for replication
looks fragile. Since they are used also by lwlock, the value of them can be
changed unexpectedly. Instead, how about defining dedicated variables for
replication?

+        else if (WaitingForSyncRep)
+        {
+            /*
+             * This must NOT be a FATAL message. We want the state of the
+             * transaction being aborted to be indeterminate to ensure that
+             * the transaction completion guarantee is never broken.
+             */

The backend can reach this code path after returning the commit to the client.
Instead, how about doing this in EndCommand, to close the connection before
returning the commit?

+        LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
+        sync_priority = walsnd->sync_standby_priority;
+        LWLockRelease(SyncRepLock);

LW_SHARE can be used here, instead.

+            /*
+             * Wait no longer if we have already reached our LSN
+             */
+            if (XLByteLE(XactCommitLSN, queue->lsn))
+            {
+                /* No need to wait */
+                LWLockRelease(SyncRepLock);
+                return;
+            }

It might take long to acquire SyncRepLock, so how about comparing
our LSN with WalSnd->flush before here?

replication_timeout_client depends on GetCurrentTransactionStopTimestamp().
In COMMIT case, it's OK. But In PREPARE TRANSACTION, COMMIT PREPARED
and ROLLBACK PREPARED cases, it seems problematic because they don't call
SetCurrentTransactionStopTimestamp().

In SyncRepWaitOnQueue, the backend can theoretically call WaitLatch() again
after the wake-up from the latch. In this case, the "timeout" should
be calculated
again. Otherwise, it would take unexpectedly very long to cause the timeout.

Regards,

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