Add ability to disable sync replication when there're not enough ready standbys.

[rg2011]

Fixes issue #330 by adding support for MinSynchronousStandbys == 0, which removes fake sync replicas from the replica list.

Changes to cluster config:

• Removed check for MinSynchronousStandbys > 0

Changes to sentinel:

• Skip adding the fakeStandbyName sync standby when the available standbys are 0.

Changes to ha_test:

• Duplicated tests that depend on sync replication stopping writes when the sync replica is down.

[rg2011]

Last test run errors look like etcd timeouts, unrelated to the code itself:

proxy_test.go:164: error waiting on store up: timeout

Could someone with permissions please rerun/confirm?

[sgotti]

@rg2011 Thanks for your PR. #330 was a very old discussion and many things in the code and sentinel logic changed from that time. So:

• I'm not sure I can totally understand the changes to the sentinel logic but they affect multiple parts. Won't be possible to just allow setting MinSynchronousStandbys to 0 without doing any change to the sentinel logic and avoiding adding a new option (strictSyncRepl)?

[rg2011]

@rg2011 Thanks for your PR. #330 was a very old discussion and many things in the code and sentinel logic changed from that time. So:

* I'm not sure I can totally understand the changes to the sentinel logic but they affect multiple parts. Won't be possible to just allow setting MinSynchronousStandbys to 0 without doing any change to the sentinel logic and avoiding adding a new option (strictSyncRepl)?

Hi, thanks for reviewing a PR for such an old issue :D. The extra logic is mostly for avoiding keeping previous / unhealthy standbys in the list of syncStandbys, to speed up failover from sync to async. But honestly I have not tested what's the actual speedup, if any. so I'm ok with trying just MinSynchronousStandbys = 0 first, let me update this.

[rg2011]

Hi @sgotti . Finished updating the PR and the tests passed, so I think it is ready for review.

[sgotti]

I'll add a note about the behavior when the value is 0.

[rg2011]

See 21f5e31

[sgotti]

@rg2011 Thanks! Great work. Just a little request on adding a note about minSynchronousStandbys == 0 in the doc. Can you please squash in a single commit?

[rg2011]

Squished into a single commit in #843. Please move discussion to that PR.

[rg2011]

Learnt how to do the rebase on the same branch :) . Rebased to a single commit.

[sgotti]

Learnt how to do the rebase on the same branch :) . Rebased to a single commit.

Thanks. After thinking a bit more about this, someone could want to have MinSynchronousStandbys to a number greater than one and disable sync repl when there're not enough ready standbys. So a strictSyncRepl parameter instead of accepting MinSynchronousStandbys == 0 could be better. I think we can do this without many changes to the sentinel logic like in the first PR iteration, just when choosing when to add a fake standby or not like in your last PR.

You should also rename the commit to something like:

*: Add option to disable sync replication when there're not enough ready standbys.

[rg2011]

Thanks. After thinking a bit more about this, someone could want to have MinSynchronousStandbys to a number greater than one and disable sync repl when there're not enough ready standbys. So a strictSyncRepl parameter instead of accepting MinSynchronousStandbys == 0 could be better. I think we can do this without many changes to the sentinel logic like in the first PR iteration, just when choosing when to add a fake standby or not like in your last PR.

You should also rename the commit to something like:

*: Add option to disable sync replication when there're not enough ready standbys.

I'm not sure about that use case. As long as there are ready replicas to add to the sync list, the sentinel will target for MaxSynchronousStandbys, it won't stop at MinSynchronousStandbys. So I don't see the benefit in setting MinSynchronousStandbys to anything higher than the absolute minimum number of sync replicas you will tolerate in a worst case scenario. Unless we change the sentinel logic, then a flag would make sense.

[sgotti]

I'm not sure about that use case. As long as there are ready replicas to add to the sync list, the sentinel will target for MaxSynchronousStandbys, it won't stop at MinSynchronousStandbys. So I don't see the benefit in setting MinSynchronousStandbys to anything higher than the absolute minimum number of sync replicas you will tolerate in a worst case scenario. Unless we change the sentinel logic, then a flag would make sense.

Yeah, it's probably a not really common use case, so we can go ahead with the current approach and improve it in future if someone wants to implement it.

So just please add some doc on the behavior of MinSynchronousStandbys == 0 in the documentation and rename the commit to something like:

*: Add ability to disable sync replication when there're not enough ready standbys.

[rg2011]

So just please add some doc on the behavior of MinSynchronousStandbys == 0 in the documentation and rename the commit to something like:

*: Add ability to disable sync replication when there're not enough ready standbys.

Added a brief note about MinSynchronousStandbys == 0 in 21f5e31 . Didn't squash and rename the commit yet, in case you think we should add or remove something else from the doc.

[sgotti]

@rg2011 Docs seems good, can you please also some brief note on the behavior of MinSynchronousStandbys == 0 in doc/cluster_spec.md and then squash and rename the commit? Thanks.

[rg2011]

Updated doc/cluster_spec.md and squashed. Glad to see this making it into milestone v0.18.0. Thanks!

[sgotti]

@rg2011 Thanks a lot! LGTM. Merging.