Re: SyncRepLock acquired exclusively in default configuration - Mailing list pgsql-hackers

On Fri, 28 Aug 2020 at 10:33, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2020/08/27 15:59, Asim Praveen wrote:
> >
> >> On 26-Aug-2020, at 11:10 PM, Fujii Masao <masao.fujii@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