Re: SyncRepLock acquired exclusively in default configuration

On 2020/08/27 15:59, Asim Praveen wrote:
>
>> 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.”

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.

>
>>
>> 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/

Fixed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION