Re: SyncRepLock acquired exclusively in default configuration

The proposed fix looks good, it resolves the lock contention problem as intended. +1 from my side.

> On 09-Jul-2020, at 4:22 PM, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 
> 
> Regarding how to fix, don't we need memory barrier when reading
> sync_standbys_defined? Without that, after SyncRepUpdateSyncStandbysDefined()
> updates it to true, SyncRepWaitForLSN() can see the previous value,
> i.e., false, and then exit out of the function. Is this right?
> If this is right, we need memory barrier to avoid this issue?
> 

There is no out-of-order execution hazard in the scenario you are describing, memory barriers don’t seem to fit.  Using locks to synchronise checkpointer process and a committing backend process is the right way.  We have made a conscious decision to bypass the lock, which looks correct in this case.

As an aside, there is a small (?) window where a change to synchronous_standby_names GUC is partially propagated among committing backends, checkpointer and walsender.  Such a window may result in walsender declaring a standby as synchronous while a commit backend fails to wait for it in SyncRepWaitForLSN.  The root cause is walsender uses sync_standby_priority, a per-walsender variable to tell if a standby is synchronous.  It is updated when walsender processes a config change.  Whereas sync_standbys_defined, a variable updated by checkpointer, is used by committing backends to determine if they need to wait.  If checkpointer is busy flushing buffers, it may take longer than walsender to reflect a change in sync_standbys_defined.  This is a low impact problem, should be ok to live with it.

Asim