On Tue, 7 Apr 2020 at 06:14, Andres Freund <and...@anarazel.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.saw...@2ndquadrant.com> wrote: > > > On Mon, 6 Apr 2020 at 14:04, Andres Freund <and...@anarazel.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