Avoid unnecessary acquisition of SyncRepLock in transaction commit time.

In SyncRepWaitForLSN() routine called in transaction commit time,
SyncRepLock is necessary to atomically both check the shared
sync_standbys_defined flag and operate the sync replication wait-queue.
On the other hand, when the flag is false, the lock is not necessary
because the wait-queue is not touched. But due to the changes by
commit 48c9f49265, previously the lock was taken whatever the flag was.
This could cause unnecessary performance overhead in every transaction
commit time. Therefore this commit avoids that unnecessary aquisition
of SyncRepLock.

Author: Fujii Masao
Reviewed-by: Asim Praveen, Masahiko Sawada,
Discussion: https://postgr.es/m/20200406050332.nsscfqjzk2d57zyx@alap3.anarazel.de

diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index df1e341c7642ab8e2b06d2acd46349045b6efc6f..6e8c76537af0a02ae15966a66d059ef0bb09a18b 100644 (file)
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -158,18 +158,30 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
     */
    Assert(InterruptHoldoffCount > 0);
 
+   /*
+    * Fast exit if user has not requested sync replication, or there are no
+    * sync replication standby names defined.
+    *
+    * Since this routine gets called every commit time, it's important to
+    * exit quickly if sync replication is not requested. So we check
+    * WalSndCtl->sync_standbys_defined flag without the lock and exit
+    * immediately if it's false. If it's true, we need to check it again later
+    * while holding the lock, to check the flag and operate the sync rep
+    * queue atomically. This is necessary to avoid the race condition
+    * described in SyncRepUpdateSyncStandbysDefined(). On the other
+    * hand, if it's false, the lock is not necessary because we don't touch
+    * the queue.
+    */
+   if (!SyncRepRequested() ||
+       !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
+       return;
+
    /* Cap the level for anything other than commit to remote flush only. */
    if (commit)
        mode = SyncRepWaitMode;
    else
        mode = Min(SyncRepWaitMode, SYNC_REP_WAIT_FLUSH);
 
-   /*
-    * Fast exit if user has not requested sync replication.
-    */
-   if (!SyncRepRequested())
-       return;
-
    Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
    Assert(WalSndCtl != NULL);