Re: SyncRepLock acquired exclusively in default configuration

On 2020/04/08 3:01, Ashwin Agrawal wrote:
> 
> On Mon, Apr 6, 2020 at 2:14 PM Andres Freund <andres@anarazel.de <mailto:andres@anarazel.de>> wrote:
> 
>      > How about we change it to this ?
> 
>     Hm. Better. But I think it might need at least a compiler barrier /
>     volatile memory load?  Unlikely here, but otherwise the compiler could
>     theoretically just stash the variable somewhere locally (it's not likely
>     to be a problem because it'd not be long ago that we acquired an lwlock,
>     which is a full barrier).
> 
> 
> That's the part, I am not fully sure about. But reading the comment above SyncRepUpdateSyncStandbysDefined(), it seems fine.
> 
>      > Bring back the check which existed based on GUC but instead of just blindly
>      > returning based on just GUC not being set, check
>      > WalSndCtl->sync_standbys_defined. Thoughts?
> 
>     Hm. Is there any reason not to just check
>     WalSndCtl->sync_standbys_defined? rather than both !SyncStandbysDefined()
>     and WalSndCtl->sync_standbys_defined?
> 
> 
> Agree, just checking for WalSndCtl->sync_standbys_defined seems fine.

So the consensus is something like the following? Patch attached.

         /*
-        * Fast exit if user has not requested sync replication.
+        * Fast exit if user has not requested sync replication, or there are no
+        * sync replication standby names defined.
          */
-       if (!SyncRepRequested())
+       if (!SyncRepRequested() ||
+               !((volatile WalSndCtlData *) WalSndCtl)->sync_standbys_defined)
                 return;

Regards,

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