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(dot)fujii(at)oss(dot)nttdata(dot)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