Subject: Re: Race condition in SyncRepGetSyncStandbysPriority
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, tgl@sss.pgh.pa.us
Cc: masahiko.sawada@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
References: 
 <CA+fd4k7_wcpZSwK0Axq5ag1Hhb7cMgp2b_CbdZegRTSzZtQAFA@mail.gmail.com>
 <20200416.182626.526651137261602571.horikyota.ntt@gmail.com>
 <23215.1587051546@sss.pgh.pa.us>
 <20200417.145834.1847060913969327935.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <3f58063d-2544-aea8-07aa-7e0c35484187@oss.nttdata.com>
Date: Fri, 17 Apr 2020 16:03:34 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200417.145834.1847060913969327935.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2020/04/17 14:58, Kyotaro Horiguchi wrote:
> At Thu, 16 Apr 2020 11:39:06 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
>> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>>> [ syncrep-fixes-4.patch ]
>>
>> I agree that we could probably improve the clarity of this code with
>> further rewriting, but I'm still very opposed to the idea of having
>> callers know that the first num_sync array elements are the active
>> ones.  It's wrong (or at least different from current behavior) for
>> quorum mode, where there might be more than num_sync walsenders to
>> consider.  And it might not generalize very well to other syncrep
>> selection rules we might add in future, which might also not have
>> exactly num_sync interesting walsenders.  So I much prefer an API
>> definition that uses bool flags in an array that has no particular
>> ordering (so far as the callers know, anyway).  If you don't like
>> is_sync_standby, how about some more-neutral name like is_active
>> or is_interesting or include_position?
> 
> I'm convinced that each element has is_sync_standby.  I agree to the
> name is_sync_standby since I don't come up with a better name.
> 
>> I dislike the proposed comment revisions in SyncRepReleaseWaiters,
>> too, particularly the change to say that what we're "announcing"
>> is the ability to release waiters.  You did not change the actual
>> log messages, and you would have gotten a lot of pushback if
>> you tried, because the current messages make sense to users and
>> something like that would not.  But by the same token this new
>> comment isn't too helpful to somebody reading the code.
> 
> The current log messages look perfect to me.  I don't insist on the
> comment change since I might take the definition of "sync standby" too
> strictly.
> 
>> (Actually, I wonder why we even have the restriction that only
>> sync standbys can release waiters.  It's not like they are
>> going to get different results from SyncRepGetSyncRecPtr than
>> any other walsender would.  Maybe we should just drop all the
>> am_sync logic?)
> 
> I thought the same thing, though I didn't do that in the last patch.
> 
> am_sync seems intending to reduce spurious wakeups but actually
> spurious wakeup won't increase even without it.  Thus the only
> remaining task of am_sync is the trigger for the log messages and that
> fact is the sign that the log messages should be emitted within
> SyncRepGetSyncRecPtr. That eliminates references to SyncRepConfig in
> SyncRepReleaseWaiters, which make me feel ease.
> 
> The attached is baed on syncrep-fixes-1.patch + am_sync elimination.

I agree that it might be worth considering the removal of am_sync for
the master branch or v14. But I think that it should not be back-patched.

Regards,

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