Re: SyncRepLock acquired exclusively in default configuration

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

> On Mon, 6 Apr 2020 at 14:04, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > commit 48c9f4926562278a2fd2b85e7486c6d11705f177
> > Author: Simon Riggs <simon(at)2ndQuadrant(dot)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?