Re: SyncRepLock acquired exclusively in default configuration

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.

> How about we change it to this ?

Hm. Better. But I think it might need at least a compiler barrier /
volatile memory load? Unlikely here, but otherwise the compiler could
theoretically just stash the variable somewhere locally (it's not likely
to be a problem because it'd not be long ago that we acquired an lwlock,
which is a full barrier).

> 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?

Hm. Is there any reason not to just check
WalSndCtl->sync_standbys_defined? rather than both !SyncStandbysDefined()
and WalSndCtl->sync_standbys_defined?

Greetings,

Andres Freund