> On 28-Aug-2020, at 7:03 AM, Fujii Masao <masao.fu...@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
