On Thu, Mar 3, 2011 at 7:53 PM, Simon Riggs <si...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers