Re: SyncRepLock acquired exclusively in default configuration

On Mon, Apr 6, 2020 at 1:52 AM Masahiko Sawada <
masahiko.sawada@2ndquadrant.com> wrote:

> On Mon, 6 Apr 2020 at 14:04, Andres Freund <andres@anarazel.de> wrote:
> >
> > commit 48c9f4926562278a2fd2b85e7486c6d11705f177
> > Author: Simon Riggs <simon@2ndQuadrant.com>
> > Date:   2017-12-29 14:30:33 +0000
> >
> >     Fix race condition when changing synchronous_standby_names
> >
> >     A momentary window exists when synchronous_standby_names
> >     changes that allows commands issued after the change to
> >     continue to act as async until the change becomes visible.
> >     Remove the race by using more appropriate test in syncrep.c
> >
> >     Author: Asim Rama Praveen and Ashwin Agrawal
> >     Reported-by: Xin Zhang, Ashwin Agrawal, and Asim Rama Praveen
> >     Reviewed-by: Michael Paquier, Masahiko Sawada
> >
> > As far as I can tell there was no discussion about the added contention
> > due this change in the relevant thread [1].
> >
> > The default configuration has an empty synchronous_standby_names. Before
> > this change we'd fall out of SyncRepWaitForLSN() before acquiring
> > SyncRepLock in exlusive mode. Now we don't anymore.
> >
> >
> > I'm really not ok with unneccessarily adding an exclusive lock
> > acquisition to such a crucial path.
> >
>
> I think we can acquire SyncRepLock in share mode once to check
> WalSndCtl->sync_standbys_defined and if it's true then check it again
> after acquiring it in exclusive mode. But it in turn ends up with
> adding one extra LWLockAcquire and LWLockRelease in sync rep path.
>

How about we change it to this ?

diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index ffd5b31eb2..cdb82a8b28 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -165,8 +165,11 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
        /*
         * Fast exit if user has not requested sync replication.
         */
-       if (!SyncRepRequested())
-               return;
+       if (!SyncRepRequested() || !SyncStandbysDefined())
+       {
+               if (!WalSndCtl->sync_standbys_defined)
+                       return;
+       }

        Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
        Assert(WalSndCtl != NULL);

Bring back the check which existed based on GUC but instead of just blindly
returning based on just GUC not being set, check
WalSndCtl->sync_standbys_defined. Thoughts?