Re: Race condition in SyncRepGetSyncStandbysPriority

On 2020/04/17 14:58, Kyotaro Horiguchi wrote:
> At Thu, 16 Apr 2020 11:39:06 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
>> Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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