Re: SyncRepLock acquired exclusively in default configuration

> On 26-Aug-2020, at 11:10 PM, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
> I added the following comments based on the suggestion by Sawada-san upthread. Thought?
>
> + * Since this routine gets called every commit time, it's important to
> + * exit quickly if sync replication is not requested. So we check
> + * WalSndCtl->sync_standbys_define without the lock and exit
> + * immediately if it's false. If it's true, we check it again later
> + * while holding the lock, to avoid the race condition described
> + * in SyncRepUpdateSyncStandbysDefined().
>

+1. May I suggest the following addition to the above comment (feel free to
rephrase / reject)?

"If sync_standbys_defined was being set from false to true and we observe it as
false, it ok to skip the wait. Replication was async and it is in the process
of being changed to sync, due to user request. Subsequent commits will observe
the change and start waiting.”

>
> Attached is the updated version of the patch. I didn't change how to
> fix the issue. But I changed the check for fast exit so that it's called
> before setting the "mode", to avoid a few cycle.
>

Looks good to me. There is a typo in the comment:

s/sync_standbys_define/sync_standbys_defined/

Asim