Re: SyncRepLock acquired exclusively in default configuration

On Tue, 7 Apr 2020 at 06:14, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2020-04-06 11:51:06 -0700, Ashwin Agrawal wrote:
> > 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:
> > > > 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.
>
> That's still too much. Adding another lwlock acquisition, where the same
> lock is acquired by all backends (contrasting e.g. to buffer locks), to
> the commit path, for the benefit of a feature that the vast majority of
> people aren't going to use, isn't good.

Agreed.

In this case it seems okay to read WalSndCtl->sync_standbys_defined
without SyncRepLock before we acquire SyncRepLock in exclusive mode.
While changing WalSndCtl->sync_standbys_defined to true, in the
current code a backend who reached SyncRepWaitForLSN() waits on
SyncRepLock, see sync_standbys_defined is true and enqueue itself.
With this change, since we don't acquire SyncRepLock to read
WalSndCtl->sync_standbys_defined these backends return without waiting
for the change of WalSndCtl->sync_standbys_defined but it would not be
a problem. Similarly, I've considered the case where changing to
false, but I think there is no problem.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services