On Fri, 28 Aug 2020 at 10:33, Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > > > > On 2020/08/27 15:59, Asim Praveen wrote: > > > >> On 26-Aug-2020, at 11:10 PM, Fujii Masao <masao.fu...@oss.nttdata.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! >
Both v2 and v3 look good to me too. IMO I'm okay with and without the last sentence. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
