Re: Support for N synchronous standby servers - take 2

On Tue, Apr 5, 2016 at 8:08 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 5 April 2016 at 11:18, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
>>
>> > 1. Header comments in syncrep.c need changes, not just additions.
>>
>> Okay, will consider this later. And I'd appreciate if you elaborate what
>> changes are necessary specifically.
>
>
> Some of the old header comments are now wrong.

Okay, will check.

>> > 2. We need tests to ensure that k >=1 and k<=N
>>
>> The changes to replication test framework was included in the patch
>> before,
>> but I excluded it from the patch because I'd like to commit the core part
>> of
>> the patch first. Will review the test part later.
>
>
> I meant tests of setting the parameters, not tests of the feature itself.

k<=0 causes an error while parsing s_s_names in current patch.

Regarding the test of k<=N, you mean that an error should be emitted
when k is larger than or equal to the number of standby names in the list?
Multiple standbys with the same name may connect to the master.
In this case, users might want to specifiy k<=N. So k<=N seems not invalid
setting.

>> > 3. There should be a WARNING if k == N to say that we don't yet provide
>> > a
>> > level to give Apply consistency. (I mean if we specify 2 (n1, n2) or
>> > 3(n1,
>> > n2, n3) etc
>>
>> Sorry I failed to get your point. Could you tell me what Apply consistency
>> and why we cannot provide it when k = N?
>>
>> > 4. How does it work?
>> > It's pretty strange, but that isn't documented anywhere. It took me a
>> > while
>> > to figure it out even though I know that code. My thought is its a lot
>> > slower than before, which is a concern when we know by definition that k
>> > >=2
>> > for the new feature. I was going to mention the fact that this code only
>> > needs to be executed by standbys mentioned in s_s_n, so we can avoid
>> > overhead and contention for async standbys (But Masahiko just mentioned
>> > that
>> > also).
>>
>> Unless I'm missing something, the patch already avoids the overhead
>> of async standbys. Please see the top of SyncRepReleaseWaiters().
>> Since async standbys exit at the beginning of SyncRepReleaseWaiters(),
>> they don't need to perform any operations that the patch adds
>> (e.g., find out which standbys are synchronous).
>
>
> I was thinking about the overhead of scanning through the full list of
> WALSenders for each message, when it is a sync standby.

This is true even in current release or before.

Regards,

--
Fujii Masao