> 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
