Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

On Tue, Apr 5, 2016 at 4:35 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 4 April 2016 at 10:35, Simon Riggs <simon@2ndquadrant.com> wrote:
>>
>> On 4 April 2016 at 09:28, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>>>
>>> Barring any objections, I'll commit this patch.
>>
>>
>> That sounds good.
>>
>> May I have one more day to review this? Actually more like 3-4 hours.
>
>
> What we have here is useful and elegant. I love the simplicity and backwards
> compatibility of the design. Very nice, chef.
>
> I am in favour of committing something for 9.6, though I do have some
> objective comments

Thanks for the review!

> 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.

> 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.

>
> 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).

> 5. Timing – k > 1 will be slower by definition and more complex to
> configure, yet there is no timing facility to measure the effect of this,
> even though we have a new timing facility in 9.6. It would be useful to have
> a track_syncrep option to keep track of typical response times from nodes.

Maybe it's useful. But it seems completely new feature, so I'm not sure
if we have enough time to push it to 9.6. Probably it's for 9.7.

> 6. Meaning of k (n1, n2, n3) with N servers
>
> It's clearly documented that this means k replies IN SEQUENCE. I believe the
> typical meaning of would be “any k out of N”, which would be faster than
> what we have, e.g.
>    3 (n1, n2, n3) would release as soon as (n1, n2) or (n2, n3) or (n1, n3)
> acknowledge.
>
> The “any k” option is not currently possible, but could be fairly easily.
> The syntax should also be easily extensible.
>
> I would call what we have now “first” semantics, and we could have both of
> these...
>
> * first k (n1, n2, n3) – does the same as k (n1, n2, n3) does now
> * any k (n1, n2, n3) – would release waiters as soon as we have the
> responses from k out of N standbys. “any k” would be faster, so is desirable
> for performance and resilience

We discussed the syntax very long time, so restarting the discussion
and keeping the patch uncommited is not good. We might fail to commit
anything about N-sync rep in 9.6. So let's commit the current patch first
and restart the discussion later.

Regards,

--
Fujii Masao