Re: SyncRepLock acquired exclusively in default configuration

> On 28-Aug-2020, at 7:03 AM, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> 
> On 2020/08/27 15:59, Asim Praveen wrote:
>> 
>> +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.”
> 
> Thanks for the suggestion! I'm not sure if it's worth adding this because
> it seems obvious thing. But maybe you imply that we need to comment
> why the lock is not necessary when sync_standbys_defined is false. Right?
> If so, what about updating the comments as follows?
> 
> +	 * 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_defined flag without the lock and exit
> +	 * immediately if it's false. If it's true, we need to check it again later
> +	 * while holding the lock, to check the flag and operate the sync rep
> +	 * queue atomically. This is necessary to avoid the race condition
> +	 * described in SyncRepUpdateSyncStandbysDefined(). On the other
> +	 * hand, if it's false, the lock is not necessary because we don't touch
> +	 * the queue.

Thank you for updating the comment.  This looks better.

Asim