[Tarantool-patches] [PATCH v4 0/3] qsync: evaluate replication_synchro_quorum dynamically

[Tarantool-patches] [PATCH v4 0/3] qsync: evaluate replication_synchro_quorum dynamically

[Cyrill Gorcunov]

Guys, take a look please once time permit.

v2 (by Serge):
 - keep replication_synchro_quorum been skipped at bootstrap in load_cfg.lua
 - eliminate redundant say_info calls
 - call quorum update routine from replica_set_id/replica_clear_id
 - use replicaset.registered_count directly when evaluating the formula
 - make quorum evaluation procedure always return value in allowed range,
   the only error which may happen here is some syntax error or Lua evaluation
   errors
 - a test has been added

v3 (by Serge, Mons, Vlad):
 - use replica.lua in tests
 - use N symbol in formula
 - use lua_pcall when evaluating a formula
 - make formula more safe itself, provide various math helpers
 - use box_update_replication_synchro_quorum name as a general
   updater from replication code
 - do not forget to update raft election quorum inside
   box_update_replication_synchro_quorum
 - print warns inside functions evaluator if value get out of bounds

v4 (by Vlad):
 - when testing a formula we walk over all amount of replicas,
   thus we are sure that later when real evaluation takes place
   we won't get quorum out of bounds
 - improve test to make sure that when no quorum aquired the
   transaction doesn't pass

issue https://github.com/tarantool/tarantool/issues/5446
branch gorcunov/gh-5446-eval-quorum-4

Cyrill Gorcunov (3):
  cfg: add cfg_isnumber helper
  cfg: support symbolic evaluation of replication_synchro_quorum
  test: add replication/gh-5446-qsync-eval-quorum.test.lua

 src/box/box.cc                                | 147 +++++++++-
 src/box/box.h                                 |   1 +
 src/box/lua/load_cfg.lua                      |   2 +-
 src/box/replication.cc                        |   4 +-
 src/cfg.c                                     |   9 +
 src/cfg.h                                     |   6 +
 .../gh-5446-qsync-eval-quorum.result          | 268 ++++++++++++++++++
 .../gh-5446-qsync-eval-quorum.test.lua        | 107 +++++++
 test/replication/replica-quorum-1.lua         |   1 +
 test/replication/replica-quorum-2.lua         |   1 +
 test/replication/replica-quorum-3.lua         |   1 +
 test/replication/replica-quorum-4.lua         |   1 +
 test/replication/replica-quorum-5.lua         |   1 +
 test/replication/replica-quorum-6.lua         |   1 +
 14 files changed, 543 insertions(+), 7 deletions(-)
 create mode 100644 test/replication/gh-5446-qsync-eval-quorum.result
 create mode 100644 test/replication/gh-5446-qsync-eval-quorum.test.lua
 create mode 120000 test/replication/replica-quorum-1.lua
 create mode 120000 test/replication/replica-quorum-2.lua
 create mode 120000 test/replication/replica-quorum-3.lua
 create mode 120000 test/replication/replica-quorum-4.lua
 create mode 120000 test/replication/replica-quorum-5.lua
 create mode 120000 test/replication/replica-quorum-6.lua


base-commit: 28f3b2f1e845aff49048d92f9062a4dfa365bf57
-- 
2.26.2

[Tarantool-patches] [PATCH v4 1/3] cfg: add cfg_isnumber helper

[Cyrill Gorcunov]

We will need it to figure out if parameter
is a numeric value when doing configuration
check.

Part-of #5446

Acked-by: Serge Petrenko <sergepetrenko@tarantool.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/cfg.c | 9 +++++++++
 src/cfg.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/src/cfg.c b/src/cfg.c
index 46cff1999..cf07d5817 100644
--- a/src/cfg.c
+++ b/src/cfg.c
@@ -57,6 +57,15 @@ cfg_geti(const char *param)
 	return val;
 }
 
+bool
+cfg_isnumber(const char *param)
+{
+	cfg_get(param);
+	bool ret = !!lua_isnumber(tarantool_L, -1);
+	lua_pop(tarantool_L, 1);
+	return ret;
+}
+
 int
 cfg_getb(const char *param)
 {
diff --git a/src/cfg.h b/src/cfg.h
index 140bdddb8..e2955e6b2 100644
--- a/src/cfg.h
+++ b/src/cfg.h
@@ -40,6 +40,12 @@ extern "C" {
 int
 cfg_geti(const char *param);
 
+/**
+ * Test if cfg parameter is a number.
+ */
+bool
+cfg_isnumber(const char *param);
+
 /**
  * Gets boolean parameter of cfg.
  * Returns -1 in case of nil
-- 
2.26.2

[Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

When synchronous replication is used we prefer a user to specify
a quorum number, ie the number of replicas where data must be
replicated before the master node continue accepting new
transactions.

This is not very convenient since a user may not know initially
how many replicas will be used. Moreover the number of replicas
may vary dynamically. For this sake we allow to specify the
number of quorum in a symbolic way.

For example

box.cfg {
	replication_synchro_quorum = "N/2+1",
}

where `N` is a number of registered replicas in a cluster.
Once new replica attached or old one detached the number
is renewed and propagated.

Internally on each replica_set_id() and replica_clear_id(),
ie at moment when replica get registered or unregistered,
we call box_update_replication_synchro_quorum() helper which
finds out if evaluation of replication_synchro_quorum is
needed and if so we calculate new replication_synchro_quorum
value based on number of currently registered replicas. Then
we notify dependent systems such as qsync and raft to update
their guts.

Note: we do *not* change the default settings for this option,
it remains 1 by default for now. Change the default option should
be done as a separate commit once we make sure that everything is
fine.

Closes #5446

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

@TarantoolBot document
Title: Support dynamic evaluation of synchronous replication quorum

Setting `replication_synchro_quorum` option to an explicit integer
value was introduced rather for simplicity sake mostly. For example
if the cluster's size is not a constant value and new replicas are
connected in dynamically then an administrator might need to increase
the option by hands or by some other external tool.

Instead one can use a dynamic evaluation of a quorum value via formal
representation using symbol `N` as a current number of registered replicas
in a cluster.

For example the canonical definition for a quorum (ie majority
of members in a set) of `N` replicas is `N/2+1`. For such configuration
define

```
box.cfg {replication_synchro_quorum = "N/2+1"}
```

The formal statement allows to provide a flexible configuration but keep
in mind that only canonical quorum (and bigger values, say `N` for all
replicas) guarantees data reliability and various weird forms such as
`N/3+1` while allowed may lead to unexpected results.
---
 src/box/box.cc           | 147 +++++++++++++++++++++++++++++++++++++--
 src/box/box.h            |   1 +
 src/box/lua/load_cfg.lua |   2 +-
 src/box/replication.cc   |   4 +-
 4 files changed, 147 insertions(+), 7 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index a8bc3471d..b820af5d0 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -554,10 +554,119 @@ box_check_replication_sync_lag(void)
 	return lag;
 }
 
+/**
+ * Evaluate replication syncro quorum number from a formula.
+ */
+static int
+box_eval_replication_synchro_quorum(int nr_replicas)
+{
+	const char fmt[] =
+		"local expr = [[%s]]\n"
+		"local f, err = loadstring('return ('..expr..')')\n"
+		"if not f then "
+			"error(string.format('Failed to load \%\%s:"
+			"\%\%s', expr, err)) "
+		"end\n"
+		"setfenv(f, {N = %d, math = {"
+			"ceil = math.ceil,"
+			"floor = math.floor,"
+			"abs = math.abs,"
+			"random = math.random,"
+			"min = math.min,"
+			"max = math.abs,"
+			"sqrt = math.sqrt,"
+			"fmod = math.fmod,"
+		"}})\n"
+		"local res = f()\n"
+		"if type(res) ~= 'number' then\n"
+			"error('Expression should return a number')\n"
+		"end\n"
+		"return math.floor(res)\n";
+	const char *expr = cfg_gets("replication_synchro_quorum");
+	int quorum = -1;
+
+	/*
+	 * cfg_gets uses static buffer as well so we need a local
+	 * one, 1K should be enough to carry arbitrary but sane
+	 * formula.
+	 */
+	char buf[1024];
+	int len = snprintf(buf, sizeof(buf), fmt, expr,
+			   nr_replicas);
+	if (len >= (int)sizeof(buf)) {
+		diag_set(ClientError, ER_CFG,
+			 "replication_synchro_quorum",
+			 "the formula is too big");
+		return -1;
+	}
+
+	luaL_loadstring(tarantool_L, buf);
+	if (lua_pcall(tarantool_L, 0, 1, 0) != 0) {
+		diag_set(ClientError, ER_CFG,
+			 "replication_synchro_quorum",
+			 lua_tostring(tarantool_L, -1));
+		return -1;
+	}
+
+	if (lua_isnumber(tarantool_L, -1))
+		quorum = (int)lua_tonumber(tarantool_L, -1);
+	lua_pop(tarantool_L, 1);
+
+	/*
+	 * At least we should have 1 node to sync, the weird
+	 * formulas such as N-2 do not guarantee quorums thus
+	 * return an error.
+	 *
+	 * Since diag_set doesn't allow to show the valid range
+	 * lets print a warning too.
+	 */
+	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
+		say_warn("the replication_synchro_quorum formula "
+			 "is evaluated to the quorum %d for replica "
+			 "number %d, which is out of range [%d;%d]",
+			 quorum, nr_replicas, 1, VCLOCK_MAX - 1);
+		diag_set(ClientError, ER_CFG,
+			 "replication_synchro_quorum",
+			 "evaluated value is out of range");
+		return -1;
+	}
+
+	return quorum;
+}
+
 static int
 box_check_replication_synchro_quorum(void)
 {
-	int quorum = cfg_geti("replication_synchro_quorum");
+	int quorum = 0;
+
+	if (!cfg_isnumber("replication_synchro_quorum")) {
+		/*
+		 * The formula uses symbolic name 'N' as
+		 * a number of currently registered replicas.
+		 *
+		 * When we're in "checking" mode we should walk
+		 * over all possible number of replicas to make
+		 * sure the formula is correct.
+		 *
+		 * Note that currently VCLOCK_MAX is pretty small
+		 * value but if we gonna increase this limit make
+		 * sure that the cycle won't take too much time.
+		 */
+		for (int i = 1; i < VCLOCK_MAX; i++) {
+			quorum = box_eval_replication_synchro_quorum(i);
+			if (quorum < 0)
+				return -1;
+		}
+		/*
+		 * Just to make clear the number we return here doesn't
+		 * have any special meaning, only errors are matter.
+		 * The real value is dynamic and will be updated on demand.
+		 */
+		quorum = 1;
+	} else {
+		quorum = cfg_geti("replication_synchro_quorum");
+	}
+
 	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
 		diag_set(ClientError, ER_CFG, "replication_synchro_quorum",
 			 "the value must be greater than zero and less than "
@@ -910,15 +1019,45 @@ box_set_replication_sync_lag(void)
 	replication_sync_lag = box_check_replication_sync_lag();
 }
 
+/**
+ * Renew replication_synchro_quorum value if defined
+ * as a formula and we need to recalculate it.
+ */
+void
+box_update_replication_synchro_quorum(void)
+{
+	int quorum = -1;
+
+	if (!cfg_isnumber("replication_synchro_quorum")) {
+		/*
+		 * The formula has been verified already. For bootstrap
+		 * stage pass 1 as a number of replicas to sync because
+		 * we're at early stage and registering a new replica.
+		 *
+		 * This should cover the valid case where formula is plain
+		 * "N", ie all replicas are to be synchro mode.
+		 */
+		int value = MAX(1, replicaset.registered_count);
+		quorum = box_eval_replication_synchro_quorum(value);
+		if (quorum <= 0 || quorum >= VCLOCK_MAX)
+			panic("failed to eval replication_synchro_quorum");
+		say_info("update replication_synchro_quorum = %d", quorum);
+	} else {
+		quorum = cfg_geti("replication_synchro_quorum");
+	}
+
+	replication_synchro_quorum = quorum;
+	txn_limbo_on_parameters_change(&txn_limbo);
+	box_raft_update_election_quorum();
+}
+
 int
 box_set_replication_synchro_quorum(void)
 {
 	int value = box_check_replication_synchro_quorum();
 	if (value < 0)
 		return -1;
-	replication_synchro_quorum = value;
-	txn_limbo_on_parameters_change(&txn_limbo);
-	box_raft_update_election_quorum();
+	box_update_replication_synchro_quorum();
 	return 0;
 }
 
diff --git a/src/box/box.h b/src/box/box.h
index b47a220b7..c3e1a1276 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -252,6 +252,7 @@ void box_set_replication_connect_timeout(void);
 void box_set_replication_connect_quorum(void);
 void box_set_replication_sync_lag(void);
 int box_set_replication_synchro_quorum(void);
+void box_update_replication_synchro_quorum(void);
 int box_set_replication_synchro_timeout(void);
 void box_set_replication_sync_timeout(void);
 void box_set_replication_skip_conflict(void);
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 770442052..2355dbcd2 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -172,7 +172,7 @@ local template_cfg = {
     replication_timeout = 'number',
     replication_sync_lag = 'number',
     replication_sync_timeout = 'number',
-    replication_synchro_quorum = 'number',
+    replication_synchro_quorum = 'string, number',
     replication_synchro_timeout = 'number',
     replication_connect_timeout = 'number',
     replication_connect_quorum = 'number',
diff --git a/src/box/replication.cc b/src/box/replication.cc
index 931c73a37..3126d86ac 100644
--- a/src/box/replication.cc
+++ b/src/box/replication.cc
@@ -251,7 +251,7 @@ replica_set_id(struct replica *replica, uint32_t replica_id)
 	say_info("assigned id %d to replica %s",
 		 replica->id, tt_uuid_str(&replica->uuid));
 	replica->anon = false;
-	box_raft_update_election_quorum();
+	box_update_replication_synchro_quorum();
 }
 
 void
@@ -300,7 +300,7 @@ replica_clear_id(struct replica *replica)
 		assert(!replica->anon);
 		replica_delete(replica);
 	}
-	box_raft_update_election_quorum();
+	box_update_replication_synchro_quorum();
 }
 
 void
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Serge Petrenko]

14.12.2020 14:39, Cyrill Gorcunov пишет:
> When synchronous replication is used we prefer a user to specify
> a quorum number, ie the number of replicas where data must be
> replicated before the master node continue accepting new
> transactions.
>
> This is not very convenient since a user may not know initially
> how many replicas will be used. Moreover the number of replicas
> may vary dynamically. For this sake we allow to specify the
> number of quorum in a symbolic way.
>
> For example
>
> box.cfg {
> 	replication_synchro_quorum = "N/2+1",
> }
>
> where `N` is a number of registered replicas in a cluster.
> Once new replica attached or old one detached the number
> is renewed and propagated.
>
> Internally on each replica_set_id() and replica_clear_id(),
> ie at moment when replica get registered or unregistered,
> we call box_update_replication_synchro_quorum() helper which
> finds out if evaluation of replication_synchro_quorum is
> needed and if so we calculate new replication_synchro_quorum
> value based on number of currently registered replicas. Then
> we notify dependent systems such as qsync and raft to update
> their guts.
>
> Note: we do *not* change the default settings for this option,
> it remains 1 by default for now. Change the default option should
> be done as a separate commit once we make sure that everything is
> fine.
>
> Closes #5446
>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>
> @TarantoolBot document
> Title: Support dynamic evaluation of synchronous replication quorum
>
> Setting `replication_synchro_quorum` option to an explicit integer
> value was introduced rather for simplicity sake mostly. For example
> if the cluster's size is not a constant value and new replicas are
> connected in dynamically then an administrator might need to increase
> the option by hands or by some other external tool.
>
> Instead one can use a dynamic evaluation of a quorum value via formal
> representation using symbol `N` as a current number of registered replicas
> in a cluster.
>
> For example the canonical definition for a quorum (ie majority
> of members in a set) of `N` replicas is `N/2+1`. For such configuration
> define
>
> ```
> box.cfg {replication_synchro_quorum = "N/2+1"}
> ```
>
> The formal statement allows to provide a flexible configuration but keep
> in mind that only canonical quorum (and bigger values, say `N` for all
> replicas) guarantees data reliability and various weird forms such as
> `N/3+1` while allowed may lead to unexpected results.
> ---
>   src/box/box.cc           | 147 +++++++++++++++++++++++++++++++++++++--
>   src/box/box.h            |   1 +
>   src/box/lua/load_cfg.lua |   2 +-
>   src/box/replication.cc   |   4 +-
>   4 files changed, 147 insertions(+), 7 deletions(-)
>
> diff --git a/src/box/box.cc b/src/box/box.cc
> index a8bc3471d..b820af5d0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -554,10 +554,119 @@ box_check_replication_sync_lag(void)
>   	return lag;
>   }
>   
> +/**
> + * Evaluate replication syncro quorum number from a formula.
> + */
> +static int
> +box_eval_replication_synchro_quorum(int nr_replicas)
> +{
> +	const char fmt[] =
> +		"local expr = [[%s]]\n"
> +		"local f, err = loadstring('return ('..expr..')')\n"
> +		"if not f then "
> +			"error(string.format('Failed to load \%\%s:"
> +			"\%\%s', expr, err)) "
> +		"end\n"
> +		"setfenv(f, {N = %d, math = {"
> +			"ceil = math.ceil,"
> +			"floor = math.floor,"
> +			"abs = math.abs,"
> +			"random = math.random,"
> +			"min = math.min,"
> +			"max = math.abs,"


typo: math.max


Other than that, LGTM.


> +			"sqrt = math.sqrt,"
> +			"fmod = math.fmod,"
> +		"}})\n"
> +		"local res = f()\n"
> +		"if type(res) ~= 'number' then\n"
> +			"error('Expression should return a number')\n"
> +		"end\n"
> +		"return math.floor(res)\n";
> +	const char *expr = cfg_gets("replication_synchro_quorum");
> +	int quorum = -1;
> +
> +	/*
> +	 * cfg_gets uses static buffer as well so we need a local
> +	 * one, 1K should be enough to carry arbitrary but sane
> +	 * formula.
> +	 */
> +	char buf[1024];
> +	int len = snprintf(buf, sizeof(buf), fmt, expr,
> +			   nr_replicas);
> +	if (len >= (int)sizeof(buf)) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "the formula is too big");
> +		return -1;
> +	}
> +
> +	luaL_loadstring(tarantool_L, buf);
> +	if (lua_pcall(tarantool_L, 0, 1, 0) != 0) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 lua_tostring(tarantool_L, -1));
> +		return -1;
> +	}
> +
> +	if (lua_isnumber(tarantool_L, -1))
> +		quorum = (int)lua_tonumber(tarantool_L, -1);
> +	lua_pop(tarantool_L, 1);
> +
> +	/*
> +	 * At least we should have 1 node to sync, the weird
> +	 * formulas such as N-2 do not guarantee quorums thus
> +	 * return an error.
> +	 *
> +	 * Since diag_set doesn't allow to show the valid range
> +	 * lets print a warning too.
> +	 */
> +	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
> +		say_warn("the replication_synchro_quorum formula "
> +			 "is evaluated to the quorum %d for replica "
> +			 "number %d, which is out of range [%d;%d]",
> +			 quorum, nr_replicas, 1, VCLOCK_MAX - 1);
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "evaluated value is out of range");
> +		return -1;
> +	}
> +
> +	return quorum;
> +}
> +
>   static int
>   box_check_replication_synchro_quorum(void)
>   {
> -	int quorum = cfg_geti("replication_synchro_quorum");
> +	int quorum = 0;
> +
> +	if (!cfg_isnumber("replication_synchro_quorum")) {
> +		/*
> +		 * The formula uses symbolic name 'N' as
> +		 * a number of currently registered replicas.
> +		 *
> +		 * When we're in "checking" mode we should walk
> +		 * over all possible number of replicas to make
> +		 * sure the formula is correct.
> +		 *
> +		 * Note that currently VCLOCK_MAX is pretty small
> +		 * value but if we gonna increase this limit make
> +		 * sure that the cycle won't take too much time.
> +		 */
> +		for (int i = 1; i < VCLOCK_MAX; i++) {
> +			quorum = box_eval_replication_synchro_quorum(i);
> +			if (quorum < 0)
> +				return -1;
> +		}
> +		/*
> +		 * Just to make clear the number we return here doesn't
> +		 * have any special meaning, only errors are matter.
> +		 * The real value is dynamic and will be updated on demand.
> +		 */
> +		quorum = 1;
> +	} else {
> +		quorum = cfg_geti("replication_synchro_quorum");
> +	}
> +
>   	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
>   		diag_set(ClientError, ER_CFG, "replication_synchro_quorum",
>   			 "the value must be greater than zero and less than "
> @@ -910,15 +1019,45 @@ box_set_replication_sync_lag(void)
>   	replication_sync_lag = box_check_replication_sync_lag();
>   }
>   
> +/**
> + * Renew replication_synchro_quorum value if defined
> + * as a formula and we need to recalculate it.
> + */
> +void
> +box_update_replication_synchro_quorum(void)
> +{
> +	int quorum = -1;
> +
> +	if (!cfg_isnumber("replication_synchro_quorum")) {
> +		/*
> +		 * The formula has been verified already. For bootstrap
> +		 * stage pass 1 as a number of replicas to sync because
> +		 * we're at early stage and registering a new replica.
> +		 *
> +		 * This should cover the valid case where formula is plain
> +		 * "N", ie all replicas are to be synchro mode.
> +		 */
> +		int value = MAX(1, replicaset.registered_count);
> +		quorum = box_eval_replication_synchro_quorum(value);
> +		if (quorum <= 0 || quorum >= VCLOCK_MAX)
> +			panic("failed to eval replication_synchro_quorum");
> +		say_info("update replication_synchro_quorum = %d", quorum);
> +	} else {
> +		quorum = cfg_geti("replication_synchro_quorum");
> +	}
> +
> +	replication_synchro_quorum = quorum;
> +	txn_limbo_on_parameters_change(&txn_limbo);
> +	box_raft_update_election_quorum();
> +}
> +
>   int
>   box_set_replication_synchro_quorum(void)
>   {
>   	int value = box_check_replication_synchro_quorum();
>   	if (value < 0)
>   		return -1;
> -	replication_synchro_quorum = value;
> -	txn_limbo_on_parameters_change(&txn_limbo);
> -	box_raft_update_election_quorum();
> +	box_update_replication_synchro_quorum();
>   	return 0;
>   }
>   
> diff --git a/src/box/box.h b/src/box/box.h
> index b47a220b7..c3e1a1276 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -252,6 +252,7 @@ void box_set_replication_connect_timeout(void);
>   void box_set_replication_connect_quorum(void);
>   void box_set_replication_sync_lag(void);
>   int box_set_replication_synchro_quorum(void);
> +void box_update_replication_synchro_quorum(void);
>   int box_set_replication_synchro_timeout(void);
>   void box_set_replication_sync_timeout(void);
>   void box_set_replication_skip_conflict(void);
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 770442052..2355dbcd2 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -172,7 +172,7 @@ local template_cfg = {
>       replication_timeout = 'number',
>       replication_sync_lag = 'number',
>       replication_sync_timeout = 'number',
> -    replication_synchro_quorum = 'number',
> +    replication_synchro_quorum = 'string, number',
>       replication_synchro_timeout = 'number',
>       replication_connect_timeout = 'number',
>       replication_connect_quorum = 'number',
> diff --git a/src/box/replication.cc b/src/box/replication.cc
> index 931c73a37..3126d86ac 100644
> --- a/src/box/replication.cc
> +++ b/src/box/replication.cc
> @@ -251,7 +251,7 @@ replica_set_id(struct replica *replica, uint32_t replica_id)
>   	say_info("assigned id %d to replica %s",
>   		 replica->id, tt_uuid_str(&replica->uuid));
>   	replica->anon = false;
> -	box_raft_update_election_quorum();
> +	box_update_replication_synchro_quorum();
>   }
>   
>   void
> @@ -300,7 +300,7 @@ replica_clear_id(struct replica *replica)
>   		assert(!replica->anon);
>   		replica_delete(replica);
>   	}
> -	box_raft_update_election_quorum();
> +	box_update_replication_synchro_quorum();
>   }
>   
>   void

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

On Wed, Dec 16, 2020 at 04:21:31PM +0300, Serge Petrenko wrote:
> > +		"setfenv(f, {N = %d, math = {"
> > +			"ceil = math.ceil,"
> > +			"floor = math.floor,"
> > +			"abs = math.abs,"
> > +			"random = math.random,"
> > +			"min = math.min,"
> > +			"max = math.abs,"
> 
> 
> typo: math.max 
> 
> Other than that, LGTM.

Good catch, thanks! A fix is forcepushed.

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> @TarantoolBot document
> Title: Support dynamic evaluation of synchronous replication quorum
> 
> Setting `replication_synchro_quorum` option to an explicit integer
> value was introduced rather for simplicity sake mostly. For example
> if the cluster's size is not a constant value and new replicas are
> connected in dynamically then an administrator might need to increase
> the option by hands or by some other external tool.
> 
> Instead one can use a dynamic evaluation of a quorum value via formal
> representation using symbol `N` as a current number of registered replicas
> in a cluster.
> 
> For example the canonical definition for a quorum (ie majority
> of members in a set) of `N` replicas is `N/2+1`. For such configuration
> define
> 
> ```
> box.cfg {replication_synchro_quorum = "N/2+1"}
> ```
> 
> The formal statement allows to provide a flexible configuration but keep
> in mind that only canonical quorum (and bigger values, say `N` for all
> replicas) guarantees data reliability and various weird forms such as
> `N/3+1` while allowed may lead to unexpected results.

Now the description is good.

See 3 comments below.

> ---
>  src/box/box.cc           | 147 +++++++++++++++++++++++++++++++++++++--
>  src/box/box.h            |   1 +
>  src/box/lua/load_cfg.lua |   2 +-
>  src/box/replication.cc   |   4 +-
>  4 files changed, 147 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index a8bc3471d..b820af5d0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -554,10 +554,119 @@ box_check_replication_sync_lag(void)
>  	return lag;
>  }
>  
> +/**
> + * Evaluate replication syncro quorum number from a formula.
> + */
> +static int
> +box_eval_replication_synchro_quorum(int nr_replicas)

1. I see you decided to never pass 0 here. Then I suggest to
add an assertion nr_replicas > 0 and < VCLOCK_MAX.

> +{
> +	const char fmt[] =
> +		"local expr = [[%s]]\n"
> +		"local f, err = loadstring('return ('..expr..')')\n"
> +		"if not f then "
> +			"error(string.format('Failed to load \%\%s:"
> +			"\%\%s', expr, err)) "
> +		"end\n"
> +		"setfenv(f, {N = %d, math = {"
> +			"ceil = math.ceil,"
> +			"floor = math.floor,"
> +			"abs = math.abs,"
> +			"random = math.random,"
> +			"min = math.min,"
> +			"max = math.abs,"
> +			"sqrt = math.sqrt,"
> +			"fmod = math.fmod,"
> +		"}})\n"
> +		"local res = f()\n"
> +		"if type(res) ~= 'number' then\n"
> +			"error('Expression should return a number')\n"
> +		"end\n"
> +		"return math.floor(res)\n";
> +	const char *expr = cfg_gets("replication_synchro_quorum");
> +	int quorum = -1;
> +
> +	/*
> +	 * cfg_gets uses static buffer as well so we need a local
> +	 * one, 1K should be enough to carry arbitrary but sane
> +	 * formula.
> +	 */
> +	char buf[1024];
> +	int len = snprintf(buf, sizeof(buf), fmt, expr,
> +			   nr_replicas);
> +	if (len >= (int)sizeof(buf)) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "the formula is too big");
> +		return -1;
> +	}
> +
> +	luaL_loadstring(tarantool_L, buf);
> +	if (lua_pcall(tarantool_L, 0, 1, 0) != 0) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 lua_tostring(tarantool_L, -1));
> +		return -1;
> +	}
> +
> +	if (lua_isnumber(tarantool_L, -1))
> +		quorum = (int)lua_tonumber(tarantool_L, -1);
> +	lua_pop(tarantool_L, 1);
> +
> +	/*
> +	 * At least we should have 1 node to sync, the weird
> +	 * formulas such as N-2 do not guarantee quorums thus
> +	 * return an error.
> +	 *
> +	 * Since diag_set doesn't allow to show the valid range
> +	 * lets print a warning too.

2. Specifically for such cases we use tt_sprintf(). See usage
examples throughout the sources.

> +	 */
> +	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
> +		say_warn("the replication_synchro_quorum formula "
> +			 "is evaluated to the quorum %d for replica "
> +			 "number %d, which is out of range [%d;%d]",
> +			 quorum, nr_replicas, 1, VCLOCK_MAX - 1);
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "evaluated value is out of range");
> +		return -1;
> +	}
> +
> +	return quorum;
> +}
> +
>  static int
>  box_check_replication_synchro_quorum(void)
>  {
> -	int quorum = cfg_geti("replication_synchro_quorum");
> +	int quorum = 0;
> +
> +	if (!cfg_isnumber("replication_synchro_quorum")) {
> +		/*
> +		 * The formula uses symbolic name 'N' as
> +		 * a number of currently registered replicas.
> +		 *
> +		 * When we're in "checking" mode we should walk
> +		 * over all possible number of replicas to make
> +		 * sure the formula is correct.
> +		 *
> +		 * Note that currently VCLOCK_MAX is pretty small
> +		 * value but if we gonna increase this limit make
> +		 * sure that the cycle won't take too much time.
> +		 */
> +		for (int i = 1; i < VCLOCK_MAX; i++) {
> +			quorum = box_eval_replication_synchro_quorum(i);
> +			if (quorum < 0)
> +				return -1;
> +		}
> +		/*
> +		 * Just to make clear the number we return here doesn't
> +		 * have any special meaning, only errors are matter.
> +		 * The real value is dynamic and will be updated on demand.
> +		 */

3. Wtf? This function before your patch was supposed to return the
new quorum value. Like all cfg 'check()' functions. If it can't do that
now (but it can - just evaluate with the current number of replicas),
then the function must return only 0 or -1, like all 'binary result'
functions. Now it simply returns some random number in case the quorum
is an expression.

> +		quorum = 1;
> +	} else {
> +		quorum = cfg_geti("replication_synchro_quorum");
> +	}
> +
>  	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
>  		diag_set(ClientError, ER_CFG, "replication_synchro_quorum",
>  			 "the value must be greater than zero and less than "

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

On Fri, Dec 18, 2020 at 12:17:56AM +0100, Vladislav Shpilevoy wrote:
> >  
> > +/**
> > + * Evaluate replication syncro quorum number from a formula.
> > + */
> > +static int
> > +box_eval_replication_synchro_quorum(int nr_replicas)
> 
> 1. I see you decided to never pass 0 here. Then I suggest to
> add an assertion nr_replicas > 0 and < VCLOCK_MAX.

OK, I will force push an update.

> > +	/*
> > +	 * At least we should have 1 node to sync, the weird
> > +	 * formulas such as N-2 do not guarantee quorums thus
> > +	 * return an error.
> > +	 *
> > +	 * Since diag_set doesn't allow to show the valid range
> > +	 * lets print a warning too.
> 
> 2. Specifically for such cases we use tt_sprintf(). See usage
> examples throughout the sources.

Good idea, thanks!

> >  static int
> >  box_check_replication_synchro_quorum(void)
> >  {
> > -	int quorum = cfg_geti("replication_synchro_quorum");
> > +	int quorum = 0;
> > +
> > +	if (!cfg_isnumber("replication_synchro_quorum")) {
> > +		/*
> > +		 * The formula uses symbolic name 'N' as
> > +		 * a number of currently registered replicas.
> > +		 *
> > +		 * When we're in "checking" mode we should walk
> > +		 * over all possible number of replicas to make
> > +		 * sure the formula is correct.
> > +		 *
> > +		 * Note that currently VCLOCK_MAX is pretty small
> > +		 * value but if we gonna increase this limit make
> > +		 * sure that the cycle won't take too much time.
> > +		 */
> > +		for (int i = 1; i < VCLOCK_MAX; i++) {
> > +			quorum = box_eval_replication_synchro_quorum(i);
> > +			if (quorum < 0)
> > +				return -1;
> > +		}
> > +		/*
> > +		 * Just to make clear the number we return here doesn't
> > +		 * have any special meaning, only errors are matter.
> > +		 * The real value is dynamic and will be updated on demand.
> > +		 */
> 
> 3. Wtf? This function before your patch was supposed to return the
> new quorum value. Like all cfg 'check()' functions. If it can't do that
> now (but it can - just evaluate with the current number of replicas),
> then the function must return only 0 or -1, like all 'binary result'
> functions. Now it simply returns some random number in case the quorum
> is an expression.

As I pointer in the comment there is no special meaning in the return
value, since we update the quorum on demand. Moreover once we manage
to validate the formula we call box_update_replication_synchro_quorum
which reevaluates the quorum with current number of replicas, thus
to not make same work twice I will return 0 here.
---
diff --git a/src/box/box.cc b/src/box/box.cc
index 1b7643bfd..452e1a6ec 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -560,6 +560,8 @@ box_check_replication_sync_lag(void)
 static int
 box_eval_replication_synchro_quorum(int nr_replicas)
 {
+	assert(nr_replicas > 0 && nr_replicas < VCLOCK_MAX);
+
 	const char fmt[] =
 		"local expr = [[%s]]\n"
 		"local f, err = loadstring('return ('..expr..')')\n"
@@ -621,13 +623,14 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 	 * lets print a warning too.
 	 */
 	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
-		say_warn("the replication_synchro_quorum formula "
-			 "is evaluated to the quorum %d for replica "
-			 "number %d, which is out of range [%d;%d]",
-			 quorum, nr_replicas, 1, VCLOCK_MAX - 1);
+		const char *msg =
+			tt_sprintf("the formula is evaluated "
+				   "to the quorum %d for replica "
+				   "number %d, which is out of range "
+				   "[%d;%d]",
+				   quorum, nr_replicas, 1, VCLOCK_MAX - 1);
 		diag_set(ClientError, ER_CFG,
-			 "replication_synchro_quorum",
-			 "evaluated value is out of range");
+			 "replication_synchro_quorum", msg);
 		return -1;
 	}
 
@@ -662,7 +665,7 @@ box_check_replication_synchro_quorum(void)
 		 * have any special meaning, only errors are matter.
 		 * The real value is dynamic and will be updated on demand.
 		 */
-		quorum = 1;
+		quorum = 0;
 	} else {
 		quorum = cfg_geti("replication_synchro_quorum");
 	}

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Vladislav Shpilevoy]

>>>  static int
>>>  box_check_replication_synchro_quorum(void)
>>>  {
>>> -	int quorum = cfg_geti("replication_synchro_quorum");
>>> +	int quorum = 0;
>>> +
>>> +	if (!cfg_isnumber("replication_synchro_quorum")) {
>>> +		/*
>>> +		 * The formula uses symbolic name 'N' as
>>> +		 * a number of currently registered replicas.
>>> +		 *
>>> +		 * When we're in "checking" mode we should walk
>>> +		 * over all possible number of replicas to make
>>> +		 * sure the formula is correct.
>>> +		 *
>>> +		 * Note that currently VCLOCK_MAX is pretty small
>>> +		 * value but if we gonna increase this limit make
>>> +		 * sure that the cycle won't take too much time.
>>> +		 */
>>> +		for (int i = 1; i < VCLOCK_MAX; i++) {
>>> +			quorum = box_eval_replication_synchro_quorum(i);
>>> +			if (quorum < 0)
>>> +				return -1;
>>> +		}
>>> +		/*
>>> +		 * Just to make clear the number we return here doesn't
>>> +		 * have any special meaning, only errors are matter.
>>> +		 * The real value is dynamic and will be updated on demand.
>>> +		 */
>>
>> 3. Wtf? This function before your patch was supposed to return the
>> new quorum value. Like all cfg 'check()' functions. If it can't do that
>> now (but it can - just evaluate with the current number of replicas),
>> then the function must return only 0 or -1, like all 'binary result'
>> functions. Now it simply returns some random number in case the quorum
>> is an expression.
> 
> As I pointer in the comment there is no special meaning in the return
> value, since we update the quorum on demand. Moreover once we manage
> to validate the formula we call box_update_replication_synchro_quorum
> which reevaluates the quorum with current number of replicas, thus
> to not make same work twice I will return 0 here.

I saw the comment and I understood it. But it does not mean the function
can now return random values. It does not matter if you return 0 or 1 or
whatever else. It still can't be used for anything useful except check < 0.

That makes the function result confusing and even useless.

Previously quorum value was returned *and used as quorum value*. Now it
is used only for < 0 check. Hence, why do you need to return anything
except -1 and 0?

For example, look at box_check_replication_synchro_timeout(). It returns
*timeout value*, which is used to set replication_synchro_timeout.

box_set_replication_synchro_quorum() before your patch did the same
with box_check_replication_synchro_quorum(). Now it uses check() result
only to compare it with < 0, which is *confusing* - why would it need
to return anything but -1 and 0 then?

Here is what I want to see:

====================
diff --git a/src/box/box.cc b/src/box/box.cc
index ff5c27743..751ec4733 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -640,8 +640,6 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 static int
 box_check_replication_synchro_quorum(void)
 {
-	int quorum = 0;
-
 	if (!cfg_isnumber("replication_synchro_quorum")) {
 		/*
 		 * The formula uses symbolic name 'N' as
@@ -656,8 +654,7 @@ box_check_replication_synchro_quorum(void)
 		 * sure that the cycle won't take too much time.
 		 */
 		for (int i = 1; i < VCLOCK_MAX; i++) {
-			quorum = box_eval_replication_synchro_quorum(i);
-			if (quorum < 0)
+			if (box_eval_replication_synchro_quorum(i) < 0)
 				return -1;
 		}
 		/*
@@ -666,17 +663,15 @@ box_check_replication_synchro_quorum(void)
 		 * The real value is dynamic and will be updated on demand.
 		 */
 		return 0;
-	} else {
-		quorum = cfg_geti("replication_synchro_quorum");
 	}
-
+	int quorum = cfg_geti("replication_synchro_quorum");
 	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
 		diag_set(ClientError, ER_CFG, "replication_synchro_quorum",
 			 "the value must be greater than zero and less than "
 			 "maximal number of replicas");
 		return -1;
 	}
-	return quorum;
+	return 0;
 }
 
 static double
@@ -877,7 +872,7 @@ box_check_config(void)
 	box_check_replication_connect_timeout();
 	box_check_replication_connect_quorum();
 	box_check_replication_sync_lag();
-	if (box_check_replication_synchro_quorum() < 0)
+	if (box_check_replication_synchro_quorum() != 0)
 		diag_raise();
 	if (box_check_replication_synchro_timeout() < 0)
 		diag_raise();
@@ -1057,8 +1052,7 @@ box_update_replication_synchro_quorum(void)
 int
 box_set_replication_synchro_quorum(void)
 {
-	int value = box_check_replication_synchro_quorum();
-	if (value < 0)
+	if (box_check_replication_synchro_quorum() != 0)
 		return -1;
 	box_update_replication_synchro_quorum();
 	return 0;

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

On Sun, Dec 20, 2020 at 06:01:25PM +0100, Vladislav Shpilevoy wrote:
...
> 
> I saw the comment and I understood it. But it does not mean the function
> can now return random values. It does not matter if you return 0 or 1 or
> whatever else. It still can't be used for anything useful except check < 0.
> 
> That makes the function result confusing and even useless.
> 
> Previously quorum value was returned *and used as quorum value*. Now it
> is used only for < 0 check. Hence, why do you need to return anything
> except -1 and 0?
> 
> For example, look at box_check_replication_synchro_timeout(). It returns
> *timeout value*, which is used to set replication_synchro_timeout.
> 
> box_set_replication_synchro_quorum() before your patch did the same
> with box_check_replication_synchro_quorum(). Now it uses check() result
> only to compare it with < 0, which is *confusing* - why would it need
> to return anything but -1 and 0 then?
> 
> Here is what I want to see:

Aha, I see what you mean. I think we should split then.

 - change existing code to return 0|-1
 - then introduce new code

this allows us to see the changes more clearly

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

All looks good now except 2 small comments below.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index a8bc3471d..b820af5d0 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -554,10 +554,119 @@ box_check_replication_sync_lag(void)
>  	return lag;
>  }
>  
> +/**
> + * Evaluate replication syncro quorum number from a formula.
> + */
> +static int
> +box_eval_replication_synchro_quorum(int nr_replicas)
> +{
> +	const char fmt[] =
> +		"local expr = [[%s]]\n"
> +		"local f, err = loadstring('return ('..expr..')')\n"
> +		"if not f then "
> +			"error(string.format('Failed to load \%\%s:"
> +			"\%\%s', expr, err)) "
> +		"end\n"
> +		"setfenv(f, {N = %d, math = {"
> +			"ceil = math.ceil,"
> +			"floor = math.floor,"
> +			"abs = math.abs,"
> +			"random = math.random,"
> +			"min = math.min,"
> +			"max = math.abs,"
> +			"sqrt = math.sqrt,"
> +			"fmod = math.fmod,"
> +		"}})\n"
> +		"local res = f()\n"
> +		"if type(res) ~= 'number' then\n"
> +			"error('Expression should return a number')\n"
> +		"end\n"
> +		"return math.floor(res)\n";
> +	const char *expr = cfg_gets("replication_synchro_quorum");
> +	int quorum = -1;
> +
> +	/*
> +	 * cfg_gets uses static buffer as well so we need a local
> +	 * one, 1K should be enough to carry arbitrary but sane
> +	 * formula.
> +	 */
> +	char buf[1024];
> +	int len = snprintf(buf, sizeof(buf), fmt, expr,
> +			   nr_replicas);
> +	if (len >= (int)sizeof(buf)) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 "the formula is too big");
> +		return -1;
> +	}
> +
> +	luaL_loadstring(tarantool_L, buf);
> +	if (lua_pcall(tarantool_L, 0, 1, 0) != 0) {
> +		diag_set(ClientError, ER_CFG,
> +			 "replication_synchro_quorum",
> +			 lua_tostring(tarantool_L, -1));
> +		return -1;
> +	}
> +
> +	if (lua_isnumber(tarantool_L, -1))
> +		quorum = (int)lua_tonumber(tarantool_L, -1);

1. There is a small issue:

tarantool> box.cfg{replication_synchro_quorum='4294967297'}
2020-12-21 18:33:16.015 [47366] main/103/interactive I> set 'replication_synchro_quorum' configuration option to "4294967297"
---
...

UINT32_MAX + 1 turns to 0. +2 turns to 1. So it
is accepted because you silently truncate to `int`.

> @@ -913,7 +1013,25 @@ box_set_replication_sync_lag(void)
>  void
>  box_update_replication_synchro_quorum(void)
>  {
> -	int quorum = cfg_geti("replication_synchro_quorum");
> +	int quorum = -1;
> +
> +	if (!cfg_isnumber("replication_synchro_quorum")) {
> +		/*
> +		 * The formula has been verified already. For bootstrap
> +		 * stage pass 1 as a number of replicas to sync because
> +		 * we're at early stage and registering a new replica.
> +		 *
> +		 * This should cover the valid case where formula is plain
> +		 * "N", ie all replicas are to be synchro mode.
> +		 */
> +		int value = MAX(1, replicaset.registered_count);
> +		quorum = box_eval_replication_synchro_quorum(value);
> +		if (quorum <= 0 || quorum >= VCLOCK_MAX)
> +			panic("failed to eval replication_synchro_quorum");

2. This check better be below. Because the numeric value also was
validated, right?

> +		say_info("update replication_synchro_quorum = %d", quorum);
> +	} else {
> +		quorum = cfg_geti("replication_synchro_quorum");
> +	}

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Vladislav Shpilevoy]

Oh, sorry. Also the test seems to be flaky.

I run it some number of times using this command:

	python test-run.py replication/gh-5446-qsync replication/gh-5446-qsync replication/gh-5446-qsync ... <repeat tens of times>

And got this:

[001] Test failed! Result content mismatch:
[001] --- replication/gh-5446-qsync-eval-quorum.result	Mon Dec 21 18:42:44 2020
[001] +++ var/rejects/replication/gh-5446-qsync-eval-quorum.reject	Mon Dec 21 18:47:40 2020
[001] @@ -38,7 +38,7 @@
[001]   | ...
[001]  test_run:grep_log("default", match) ~= nil
[001]   | ---
[001] - | - true
[001] + | - false
[001]   | ...
[001]  
[001]  -- Create a sync space we will operate on
[001] 

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

On Mon, Dec 21, 2020 at 06:48:04PM +0100, Vladislav Shpilevoy wrote:
> > +
> > +	if (lua_isnumber(tarantool_L, -1))
> > +		quorum = (int)lua_tonumber(tarantool_L, -1);
> 
> 1. There is a small issue:
> 
> tarantool> box.cfg{replication_synchro_quorum='4294967297'}
> 2020-12-21 18:33:16.015 [47366] main/103/interactive I> set 'replication_synchro_quorum' configuration option to "4294967297"

Actually nope. When we pass this value it is treated
as a plain number and cfg_geti trims it :(

Here is a master branch output

 | tarantool> box.cfg{replication_synchro_quorum=4294967297}
 | ...
 | 2020-12-21 22:59:00.614 [176552] main/103/interactive I> set 'replication_synchro_quorum' configuration option to 4294967297

Note that on master branch I have to pass real numebr not string,
but issue is the same...

Need to think how to deal with it.

> > +		int value = MAX(1, replicaset.registered_count);
> > +		quorum = box_eval_replication_synchro_quorum(value);
> > +		if (quorum <= 0 || quorum >= VCLOCK_MAX)
> > +			panic("failed to eval replication_synchro_quorum");
> 
> 2. This check better be below. Because the numeric value also was
> validated, right?

True, will update.

Re: [Tarantool-patches] [PATCH v4 2/3] cfg: support symbolic evaluation of replication_synchro_quorum

[Cyrill Gorcunov]

On Mon, Dec 21, 2020 at 11:02:26PM +0300, Cyrill Gorcunov wrote:
> On Mon, Dec 21, 2020 at 06:48:04PM +0100, Vladislav Shpilevoy wrote:
> > > +
> > > +	if (lua_isnumber(tarantool_L, -1))
> > > +		quorum = (int)lua_tonumber(tarantool_L, -1);
> > 
> > 1. There is a small issue:
> > 
> > tarantool> box.cfg{replication_synchro_quorum='4294967297'}
> > 2020-12-21 18:33:16.015 [47366] main/103/interactive I> set 'replication_synchro_quorum' configuration option to "4294967297"
> 
> Actually nope. When we pass this value it is treated
> as a plain number and cfg_geti trims it :(
> 
> Here is a master branch output
> 
>  | tarantool> box.cfg{replication_synchro_quorum=4294967297}
>  | ...
>  | 2020-12-21 22:59:00.614 [176552] main/103/interactive I> set 'replication_synchro_quorum' configuration option to 4294967297
> 
> Note that on master branch I have to pass real numebr not string,
> but issue is the same...
> 
> Need to think how to deal with it.

I think we might need something like below, but I'm not sure
if this won't break backward compatibility...
---
[cyrill@grain tarantool.git] git diff
diff --git a/src/cfg.c b/src/cfg.c
index 46cff1999..f896c6974 100644
--- a/src/cfg.c
+++ b/src/cfg.c
@@ -49,10 +49,18 @@ cfg_geti(const char *param)
 {
        cfg_get(param);
        int val;
-       if (lua_isboolean(tarantool_L, -1))
+       if (lua_isboolean(tarantool_L, -1)) {
                val = lua_toboolean(tarantool_L, -1);
-       else
-               val = lua_tointeger(tarantool_L, -1);
+       } else {
+               double dv = lua_tointeger(tarantool_L, -1);
+               errno = 0;
+               long long lv = llrint(dv);
+               if (errno != 0)
+                       panic("cfg_geti('%s') round failed", param);
+               if (lv > INT_MAX || lv < INT_MIN)
+                       panic("cfg_geti('%s') out of bounds", param);
+               val = (int)lv;
+       }
        lua_pop(tarantool_L, 1);
        return val;
 }
---
[cyrill@grain tarantool.git] ./src/tarantool 
Tarantool 2.7.0-112-g4c558a4ba
type 'help' for interactive help
tarantool> box.cfg{replication_synchro_quorum=4294967297}
cfg_geti('replication_synchro_quorum') out of bounds

[Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Cyrill Gorcunov]

Part-of #5446

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 .../gh-5446-qsync-eval-quorum.result          | 268 ++++++++++++++++++
 .../gh-5446-qsync-eval-quorum.test.lua        | 107 +++++++
 test/replication/replica-quorum-1.lua         |   1 +
 test/replication/replica-quorum-2.lua         |   1 +
 test/replication/replica-quorum-3.lua         |   1 +
 test/replication/replica-quorum-4.lua         |   1 +
 test/replication/replica-quorum-5.lua         |   1 +
 test/replication/replica-quorum-6.lua         |   1 +
 8 files changed, 381 insertions(+)
 create mode 100644 test/replication/gh-5446-qsync-eval-quorum.result
 create mode 100644 test/replication/gh-5446-qsync-eval-quorum.test.lua
 create mode 120000 test/replication/replica-quorum-1.lua
 create mode 120000 test/replication/replica-quorum-2.lua
 create mode 120000 test/replication/replica-quorum-3.lua
 create mode 120000 test/replication/replica-quorum-4.lua
 create mode 120000 test/replication/replica-quorum-5.lua
 create mode 120000 test/replication/replica-quorum-6.lua

diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
new file mode 100644
index 000000000..db0f8e91f
--- /dev/null
+++ b/test/replication/gh-5446-qsync-eval-quorum.result
@@ -0,0 +1,268 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'replication')
+ | ---
+ | ...
+
+-- Test syntax error
+box.cfg{replication_synchro_quorum = "aaa"}
+ | ---
+ | - error: 'Incorrect value for option ''replication_synchro_quorum'': [string "local
+ |     expr = [[aaa]]..."]:7: Expression should return a number'
+ | ...
+
+-- Test out of bounds values
+box.cfg{replication_synchro_quorum = "N+1"}
+ | ---
+ | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
+ |     is out of range'
+ | ...
+box.cfg{replication_synchro_quorum = "N-1"}
+ | ---
+ | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
+ |     is out of range'
+ | ...
+
+-- Use canonical majority formula
+box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 }
+ | ---
+ | ...
+match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+-- Create a sync space we will operate on
+_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
+ | ---
+ | ...
+s = box.space.sync
+ | ---
+ | ...
+s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
+ | ---
+ | ...
+_ = s:create_index('primary', {parts = {'id'}})
+ | ---
+ | ...
+s:insert{1, 1}
+ | ---
+ | - [1, 1]
+ | ...
+
+test_run:cmd('create server replica1 with rpl_master=default,\
+              script="replication/replica-quorum-1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica1 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+-- 1 replica -> replication_synchro_quorum = 2/2 + 1 = 2
+match = 'update replication_synchro_quorum = 2'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica2 with rpl_master=default,\
+              script="replication/replica-quorum-2.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica2 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+-- 2 replicas -> replication_synchro_quorum = 3/2 + 1 = 2
+match = 'update replication_synchro_quorum = 2'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica3 with rpl_master=default,\
+              script="replication/replica-quorum-3.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica3 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+-- 3 replicas -> replication_synchro_quorum = 4/2 + 1 = 3
+match = 'update replication_synchro_quorum = 3'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica4 with rpl_master=default,\
+              script="replication/replica-quorum-4.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica4 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+-- 4 replicas -> replication_synchro_quorum = 5/2 + 1 = 3
+match = 'update replication_synchro_quorum = 3'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica5 with rpl_master=default,\
+              script="replication/replica-quorum-5.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica5 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('create server replica6 with rpl_master=default,\
+              script="replication/replica-quorum-6.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica6 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+
+-- 6 replicas -> replication_synchro_quorum = 7/2 + 1 = 4
+match = 'update replication_synchro_quorum = 4'
+ | ---
+ | ...
+test_run:grep_log("default", match) ~= nil
+ | ---
+ | - true
+ | ...
+
+-- 5 replicas left, the commit should pass
+test_run:cmd('stop server replica1')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica1')
+ | ---
+ | - true
+ | ...
+s:insert{2, 2}
+ | ---
+ | - [2, 2]
+ | ...
+
+-- 4 replicas left,the commit should pass
+test_run:cmd('stop server replica2')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica2')
+ | ---
+ | - true
+ | ...
+s:insert{3, 3}
+ | ---
+ | - [3, 3]
+ | ...
+
+-- 3 replicas left, the commit should pass
+test_run:cmd('stop server replica3')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica3')
+ | ---
+ | - true
+ | ...
+s:insert{4, 4}
+ | ---
+ | - [4, 4]
+ | ...
+
+-- 2 replicas left, the commit should NOT pass
+--
+-- The replication_synchro_timeout set to a small value to not wait
+-- for very long for the case where we know the commit should
+-- not pass since replicas are stopped.
+box.cfg { replication_synchro_timeout = 0.5 }
+ | ---
+ | ...
+test_run:cmd('stop server replica4')
+ | ---
+ | - true
+ | ...
+s:insert{5, 5}
+ | ---
+ | - error: Quorum collection for a synchronous transaction is timed out
+ | ...
+-- restore it back and retry
+test_run:cmd('start server replica4 with wait=True, wait_load=True')
+ | ---
+ | - true
+ | ...
+box.cfg { replication_synchro_timeout = 1000 }
+ | ---
+ | ...
+s:insert{5, 5}
+ | ---
+ | - [5, 5]
+ | ...
+test_run:cmd('stop server replica4')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica4')
+ | ---
+ | - true
+ | ...
+
+-- cleanup leftovers
+
+test_run:cmd('stop server replica5')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica5')
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('stop server replica6')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica6')
+ | ---
+ | - true
+ | ...
+
+box.schema.user.revoke('guest', 'replication')
+ | ---
+ | ...
diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
new file mode 100644
index 000000000..2ecfa8c3e
--- /dev/null
+++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
@@ -0,0 +1,107 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+
+box.schema.user.grant('guest', 'replication')
+
+-- Test syntax error
+box.cfg{replication_synchro_quorum = "aaa"}
+
+-- Test out of bounds values
+box.cfg{replication_synchro_quorum = "N+1"}
+box.cfg{replication_synchro_quorum = "N-1"}
+
+-- Use canonical majority formula
+box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 }
+match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1'
+test_run:grep_log("default", match) ~= nil
+
+-- Create a sync space we will operate on
+_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
+s = box.space.sync
+s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
+_ = s:create_index('primary', {parts = {'id'}})
+s:insert{1, 1}
+
+test_run:cmd('create server replica1 with rpl_master=default,\
+              script="replication/replica-quorum-1.lua"')
+test_run:cmd('start server replica1 with wait=True, wait_load=True')
+
+-- 1 replica -> replication_synchro_quorum = 2/2 + 1 = 2
+match = 'update replication_synchro_quorum = 2'
+test_run:grep_log("default", match) ~= nil
+
+test_run:cmd('create server replica2 with rpl_master=default,\
+              script="replication/replica-quorum-2.lua"')
+test_run:cmd('start server replica2 with wait=True, wait_load=True')
+
+-- 2 replicas -> replication_synchro_quorum = 3/2 + 1 = 2
+match = 'update replication_synchro_quorum = 2'
+test_run:grep_log("default", match) ~= nil
+
+test_run:cmd('create server replica3 with rpl_master=default,\
+              script="replication/replica-quorum-3.lua"')
+test_run:cmd('start server replica3 with wait=True, wait_load=True')
+
+-- 3 replicas -> replication_synchro_quorum = 4/2 + 1 = 3
+match = 'update replication_synchro_quorum = 3'
+test_run:grep_log("default", match) ~= nil
+
+test_run:cmd('create server replica4 with rpl_master=default,\
+              script="replication/replica-quorum-4.lua"')
+test_run:cmd('start server replica4 with wait=True, wait_load=True')
+
+-- 4 replicas -> replication_synchro_quorum = 5/2 + 1 = 3
+match = 'update replication_synchro_quorum = 3'
+test_run:grep_log("default", match) ~= nil
+
+test_run:cmd('create server replica5 with rpl_master=default,\
+              script="replication/replica-quorum-5.lua"')
+test_run:cmd('start server replica5 with wait=True, wait_load=True')
+
+test_run:cmd('create server replica6 with rpl_master=default,\
+              script="replication/replica-quorum-6.lua"')
+test_run:cmd('start server replica6 with wait=True, wait_load=True')
+
+-- 6 replicas -> replication_synchro_quorum = 7/2 + 1 = 4
+match = 'update replication_synchro_quorum = 4'
+test_run:grep_log("default", match) ~= nil
+
+-- 5 replicas left, the commit should pass
+test_run:cmd('stop server replica1')
+test_run:cmd('delete server replica1')
+s:insert{2, 2}
+
+-- 4 replicas left,the commit should pass
+test_run:cmd('stop server replica2')
+test_run:cmd('delete server replica2')
+s:insert{3, 3}
+
+-- 3 replicas left, the commit should pass
+test_run:cmd('stop server replica3')
+test_run:cmd('delete server replica3')
+s:insert{4, 4}
+
+-- 2 replicas left, the commit should NOT pass
+--
+-- The replication_synchro_timeout set to a small value to not wait
+-- for very long for the case where we know the commit should
+-- not pass since replicas are stopped.
+box.cfg { replication_synchro_timeout = 0.5 }
+test_run:cmd('stop server replica4')
+s:insert{5, 5}
+-- restore it back and retry
+test_run:cmd('start server replica4 with wait=True, wait_load=True')
+box.cfg { replication_synchro_timeout = 1000 }
+s:insert{5, 5}
+test_run:cmd('stop server replica4')
+test_run:cmd('delete server replica4')
+
+-- cleanup leftovers
+
+test_run:cmd('stop server replica5')
+test_run:cmd('delete server replica5')
+
+test_run:cmd('stop server replica6')
+test_run:cmd('delete server replica6')
+
+box.schema.user.revoke('guest', 'replication')
diff --git a/test/replication/replica-quorum-1.lua b/test/replication/replica-quorum-1.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-1.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
diff --git a/test/replication/replica-quorum-2.lua b/test/replication/replica-quorum-2.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-2.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
diff --git a/test/replication/replica-quorum-3.lua b/test/replication/replica-quorum-3.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-3.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
diff --git a/test/replication/replica-quorum-4.lua b/test/replication/replica-quorum-4.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-4.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
diff --git a/test/replication/replica-quorum-5.lua b/test/replication/replica-quorum-5.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-5.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
diff --git a/test/replication/replica-quorum-6.lua b/test/replication/replica-quorum-6.lua
new file mode 120000
index 000000000..da69ac81c
--- /dev/null
+++ b/test/replication/replica-quorum-6.lua
@@ -0,0 +1 @@
+replica.lua
\ No newline at end of file
-- 
2.26.2

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Serge Petrenko]

14.12.2020 14:39, Cyrill Gorcunov пишет:
> Part-of #5446


Thanks!

LGTM.


> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   .../gh-5446-qsync-eval-quorum.result          | 268 ++++++++++++++++++
>   .../gh-5446-qsync-eval-quorum.test.lua        | 107 +++++++
>   test/replication/replica-quorum-1.lua         |   1 +
>   test/replication/replica-quorum-2.lua         |   1 +
>   test/replication/replica-quorum-3.lua         |   1 +
>   test/replication/replica-quorum-4.lua         |   1 +
>   test/replication/replica-quorum-5.lua         |   1 +
>   test/replication/replica-quorum-6.lua         |   1 +
>   8 files changed, 381 insertions(+)
>   create mode 100644 test/replication/gh-5446-qsync-eval-quorum.result
>   create mode 100644 test/replication/gh-5446-qsync-eval-quorum.test.lua
>   create mode 120000 test/replication/replica-quorum-1.lua
>   create mode 120000 test/replication/replica-quorum-2.lua
>   create mode 120000 test/replication/replica-quorum-3.lua
>   create mode 120000 test/replication/replica-quorum-4.lua
>   create mode 120000 test/replication/replica-quorum-5.lua
>   create mode 120000 test/replication/replica-quorum-6.lua
>
> diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
> new file mode 100644
> index 000000000..db0f8e91f
> --- /dev/null
> +++ b/test/replication/gh-5446-qsync-eval-quorum.result
> @@ -0,0 +1,268 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +
> +box.schema.user.grant('guest', 'replication')
> + | ---
> + | ...
> +
> +-- Test syntax error
> +box.cfg{replication_synchro_quorum = "aaa"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': [string "local
> + |     expr = [[aaa]]..."]:7: Expression should return a number'
> + | ...
> +
> +-- Test out of bounds values
> +box.cfg{replication_synchro_quorum = "N+1"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
> + |     is out of range'
> + | ...
> +box.cfg{replication_synchro_quorum = "N-1"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
> + |     is out of range'
> + | ...
> +
> +-- Use canonical majority formula
> +box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 }
> + | ---
> + | ...
> +match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +-- Create a sync space we will operate on
> +_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
> + | ---
> + | ...
> +s = box.space.sync
> + | ---
> + | ...
> +s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
> + | ---
> + | ...
> +_ = s:create_index('primary', {parts = {'id'}})
> + | ---
> + | ...
> +s:insert{1, 1}
> + | ---
> + | - [1, 1]
> + | ...
> +
> +test_run:cmd('create server replica1 with rpl_master=default,\
> +              script="replication/replica-quorum-1.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica1 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +-- 1 replica -> replication_synchro_quorum = 2/2 + 1 = 2
> +match = 'update replication_synchro_quorum = 2'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server replica2 with rpl_master=default,\
> +              script="replication/replica-quorum-2.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica2 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +-- 2 replicas -> replication_synchro_quorum = 3/2 + 1 = 2
> +match = 'update replication_synchro_quorum = 2'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server replica3 with rpl_master=default,\
> +              script="replication/replica-quorum-3.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica3 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +-- 3 replicas -> replication_synchro_quorum = 4/2 + 1 = 3
> +match = 'update replication_synchro_quorum = 3'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server replica4 with rpl_master=default,\
> +              script="replication/replica-quorum-4.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica4 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +-- 4 replicas -> replication_synchro_quorum = 5/2 + 1 = 3
> +match = 'update replication_synchro_quorum = 3'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server replica5 with rpl_master=default,\
> +              script="replication/replica-quorum-5.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica5 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('create server replica6 with rpl_master=default,\
> +              script="replication/replica-quorum-6.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server replica6 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +
> +-- 6 replicas -> replication_synchro_quorum = 7/2 + 1 = 4
> +match = 'update replication_synchro_quorum = 4'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +-- 5 replicas left, the commit should pass
> +test_run:cmd('stop server replica1')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica1')
> + | ---
> + | - true
> + | ...
> +s:insert{2, 2}
> + | ---
> + | - [2, 2]
> + | ...
> +
> +-- 4 replicas left,the commit should pass
> +test_run:cmd('stop server replica2')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica2')
> + | ---
> + | - true
> + | ...
> +s:insert{3, 3}
> + | ---
> + | - [3, 3]
> + | ...
> +
> +-- 3 replicas left, the commit should pass
> +test_run:cmd('stop server replica3')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica3')
> + | ---
> + | - true
> + | ...
> +s:insert{4, 4}
> + | ---
> + | - [4, 4]
> + | ...
> +
> +-- 2 replicas left, the commit should NOT pass
> +--
> +-- The replication_synchro_timeout set to a small value to not wait
> +-- for very long for the case where we know the commit should
> +-- not pass since replicas are stopped.
> +box.cfg { replication_synchro_timeout = 0.5 }
> + | ---
> + | ...
> +test_run:cmd('stop server replica4')
> + | ---
> + | - true
> + | ...
> +s:insert{5, 5}
> + | ---
> + | - error: Quorum collection for a synchronous transaction is timed out
> + | ...
> +-- restore it back and retry
> +test_run:cmd('start server replica4 with wait=True, wait_load=True')
> + | ---
> + | - true
> + | ...
> +box.cfg { replication_synchro_timeout = 1000 }
> + | ---
> + | ...
> +s:insert{5, 5}
> + | ---
> + | - [5, 5]
> + | ...
> +test_run:cmd('stop server replica4')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica4')
> + | ---
> + | - true
> + | ...
> +
> +-- cleanup leftovers
> +
> +test_run:cmd('stop server replica5')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica5')
> + | ---
> + | - true
> + | ...
> +
> +test_run:cmd('stop server replica6')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('delete server replica6')
> + | ---
> + | - true
> + | ...
> +
> +box.schema.user.revoke('guest', 'replication')
> + | ---
> + | ...
> diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
> new file mode 100644
> index 000000000..2ecfa8c3e
> --- /dev/null
> +++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
> @@ -0,0 +1,107 @@
> +test_run = require('test_run').new()
> +engine = test_run:get_cfg('engine')
> +
> +box.schema.user.grant('guest', 'replication')
> +
> +-- Test syntax error
> +box.cfg{replication_synchro_quorum = "aaa"}
> +
> +-- Test out of bounds values
> +box.cfg{replication_synchro_quorum = "N+1"}
> +box.cfg{replication_synchro_quorum = "N-1"}
> +
> +-- Use canonical majority formula
> +box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 }
> +match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1'
> +test_run:grep_log("default", match) ~= nil
> +
> +-- Create a sync space we will operate on
> +_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
> +s = box.space.sync
> +s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
> +_ = s:create_index('primary', {parts = {'id'}})
> +s:insert{1, 1}
> +
> +test_run:cmd('create server replica1 with rpl_master=default,\
> +              script="replication/replica-quorum-1.lua"')
> +test_run:cmd('start server replica1 with wait=True, wait_load=True')
> +
> +-- 1 replica -> replication_synchro_quorum = 2/2 + 1 = 2
> +match = 'update replication_synchro_quorum = 2'
> +test_run:grep_log("default", match) ~= nil
> +
> +test_run:cmd('create server replica2 with rpl_master=default,\
> +              script="replication/replica-quorum-2.lua"')
> +test_run:cmd('start server replica2 with wait=True, wait_load=True')
> +
> +-- 2 replicas -> replication_synchro_quorum = 3/2 + 1 = 2
> +match = 'update replication_synchro_quorum = 2'
> +test_run:grep_log("default", match) ~= nil
> +
> +test_run:cmd('create server replica3 with rpl_master=default,\
> +              script="replication/replica-quorum-3.lua"')
> +test_run:cmd('start server replica3 with wait=True, wait_load=True')
> +
> +-- 3 replicas -> replication_synchro_quorum = 4/2 + 1 = 3
> +match = 'update replication_synchro_quorum = 3'
> +test_run:grep_log("default", match) ~= nil
> +
> +test_run:cmd('create server replica4 with rpl_master=default,\
> +              script="replication/replica-quorum-4.lua"')
> +test_run:cmd('start server replica4 with wait=True, wait_load=True')
> +
> +-- 4 replicas -> replication_synchro_quorum = 5/2 + 1 = 3
> +match = 'update replication_synchro_quorum = 3'
> +test_run:grep_log("default", match) ~= nil
> +
> +test_run:cmd('create server replica5 with rpl_master=default,\
> +              script="replication/replica-quorum-5.lua"')
> +test_run:cmd('start server replica5 with wait=True, wait_load=True')
> +
> +test_run:cmd('create server replica6 with rpl_master=default,\
> +              script="replication/replica-quorum-6.lua"')
> +test_run:cmd('start server replica6 with wait=True, wait_load=True')
> +
> +-- 6 replicas -> replication_synchro_quorum = 7/2 + 1 = 4
> +match = 'update replication_synchro_quorum = 4'
> +test_run:grep_log("default", match) ~= nil
> +
> +-- 5 replicas left, the commit should pass
> +test_run:cmd('stop server replica1')
> +test_run:cmd('delete server replica1')
> +s:insert{2, 2}
> +
> +-- 4 replicas left,the commit should pass
> +test_run:cmd('stop server replica2')
> +test_run:cmd('delete server replica2')
> +s:insert{3, 3}
> +
> +-- 3 replicas left, the commit should pass
> +test_run:cmd('stop server replica3')
> +test_run:cmd('delete server replica3')
> +s:insert{4, 4}
> +
> +-- 2 replicas left, the commit should NOT pass
> +--
> +-- The replication_synchro_timeout set to a small value to not wait
> +-- for very long for the case where we know the commit should
> +-- not pass since replicas are stopped.
> +box.cfg { replication_synchro_timeout = 0.5 }
> +test_run:cmd('stop server replica4')
> +s:insert{5, 5}
> +-- restore it back and retry
> +test_run:cmd('start server replica4 with wait=True, wait_load=True')
> +box.cfg { replication_synchro_timeout = 1000 }
> +s:insert{5, 5}
> +test_run:cmd('stop server replica4')
> +test_run:cmd('delete server replica4')
> +
> +-- cleanup leftovers
> +
> +test_run:cmd('stop server replica5')
> +test_run:cmd('delete server replica5')
> +
> +test_run:cmd('stop server replica6')
> +test_run:cmd('delete server replica6')
> +
> +box.schema.user.revoke('guest', 'replication')
> diff --git a/test/replication/replica-quorum-1.lua b/test/replication/replica-quorum-1.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-1.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file
> diff --git a/test/replication/replica-quorum-2.lua b/test/replication/replica-quorum-2.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-2.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file
> diff --git a/test/replication/replica-quorum-3.lua b/test/replication/replica-quorum-3.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-3.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file
> diff --git a/test/replication/replica-quorum-4.lua b/test/replication/replica-quorum-4.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-4.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file
> diff --git a/test/replication/replica-quorum-5.lua b/test/replication/replica-quorum-5.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-5.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file
> diff --git a/test/replication/replica-quorum-6.lua b/test/replication/replica-quorum-6.lua
> new file mode 120000
> index 000000000..da69ac81c
> --- /dev/null
> +++ b/test/replication/replica-quorum-6.lua
> @@ -0,0 +1 @@
> +replica.lua
> \ No newline at end of file

-- 
Serge Petrenko

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/test/replication/gh-5446-qsync-eval-quorum.result b/test/replication/gh-5446-qsync-eval-quorum.result
> new file mode 100644
> index 000000000..db0f8e91f
> --- /dev/null
> +++ b/test/replication/gh-5446-qsync-eval-quorum.result
> @@ -0,0 +1,268 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +
> +box.schema.user.grant('guest', 'replication')
> + | ---
> + | ...
> +
> +-- Test syntax error
> +box.cfg{replication_synchro_quorum = "aaa"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': [string "local
> + |     expr = [[aaa]]..."]:7: Expression should return a number'
> + | ...
> +
> +-- Test out of bounds values
> +box.cfg{replication_synchro_quorum = "N+1"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
> + |     is out of range'
> + | ...
> +box.cfg{replication_synchro_quorum = "N-1"}
> + | ---
> + | - error: 'Incorrect value for option ''replication_synchro_quorum'': evaluated value
> + |     is out of range'
> + | ...
> +
> +-- Use canonical majority formula
> +box.cfg { replication_synchro_quorum = "N/2+1", replication_synchro_timeout = 1000 }
> + | ---
> + | ...
> +match = 'set \'replication_synchro_quorum\' configuration option to \"N\\/2%+1'
> + | ---
> + | ...
> +test_run:grep_log("default", match) ~= nil
> + | ---
> + | - true
> + | ...
> +
> +-- Create a sync space we will operate on
> +_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
> + | ---
> + | ...
> +s = box.space.sync
> + | ---
> + | ...
> +s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})

1. Why do you need the format? Why do you even need 2 fields?

> + | ---
> + | ...
> +_ = s:create_index('primary', {parts = {'id'}})

2. In primary index you can omit 'parts' - it will use the first
field automatically.

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Cyrill Gorcunov]

On Fri, Dec 18, 2020 at 12:18:00AM +0100, Vladislav Shpilevoy wrote:
> > +-- Create a sync space we will operate on
> > +_ = box.schema.space.create('sync', {is_sync = true, engine = engine})
> > + | ---
> > + | ...
> > +s = box.space.sync
> > + | ---
> > + | ...
> > +s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
> 
> 1. Why do you need the format? Why do you even need 2 fields?
> 
> > + | ---
> > + | ...
> > +_ = s:create_index('primary', {parts = {'id'}})
> 
> 2. In primary index you can omit 'parts' - it will use the first
> field automatically.

Took both notes from some existing example. Would the following be better?
---
diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
index 2ecfa8c3e..9f731a488 100644
--- a/test/replication/gh-5446-qsync-eval-quorum.test.lua
+++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
@@ -17,10 +17,8 @@ test_run:grep_log("default", match) ~= nil
 
 -- Create a sync space we will operate on
 _ = box.schema.space.create('sync', {is_sync = true, engine = engine})
-s = box.space.sync
-s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
-_ = s:create_index('primary', {parts = {'id'}})
-s:insert{1, 1}
+_ = box.space.sync:create_index('pk')
+box.space.sync:insert{1}
 
 test_run:cmd('create server replica1 with rpl_master=default,\
               script="replication/replica-quorum-1.lua"')
@@ -69,17 +67,17 @@ test_run:grep_log("default", match) ~= nil
 -- 5 replicas left, the commit should pass
 test_run:cmd('stop server replica1')
 test_run:cmd('delete server replica1')
-s:insert{2, 2}
+box.space.sync:insert{2}
 
 -- 4 replicas left,the commit should pass
 test_run:cmd('stop server replica2')
 test_run:cmd('delete server replica2')
-s:insert{3, 3}
+box.space.sync:insert{3}
 
 -- 3 replicas left, the commit should pass
 test_run:cmd('stop server replica3')
 test_run:cmd('delete server replica3')
-s:insert{4, 4}
+box.space.sync:insert{4}
 
 -- 2 replicas left, the commit should NOT pass
 --
@@ -88,11 +86,11 @@ s:insert{4, 4}
 -- not pass since replicas are stopped.
 box.cfg { replication_synchro_timeout = 0.5 }
 test_run:cmd('stop server replica4')
-s:insert{5, 5}
+box.space.sync:insert{5}
 -- restore it back and retry
 test_run:cmd('start server replica4 with wait=True, wait_load=True')
 box.cfg { replication_synchro_timeout = 1000 }
-s:insert{5, 5}
+box.space.sync:insert{5}
 test_run:cmd('stop server replica4')
 test_run:cmd('delete server replica4')
 

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Vladislav Shpilevoy]

Thanks for the fixes!

> Took both notes from some existing example. Would the following be better?

Yes, thanks, it does not raise any questions now. Although
you could keep 's' variable to access the space methods shorter.

> ---
> diff --git a/test/replication/gh-5446-qsync-eval-quorum.test.lua b/test/replication/gh-5446-qsync-eval-quorum.test.lua
> index 2ecfa8c3e..9f731a488 100644
> --- a/test/replication/gh-5446-qsync-eval-quorum.test.lua
> +++ b/test/replication/gh-5446-qsync-eval-quorum.test.lua
> @@ -17,10 +17,8 @@ test_run:grep_log("default", match) ~= nil
>  
>  -- Create a sync space we will operate on
>  _ = box.schema.space.create('sync', {is_sync = true, engine = engine})
> -s = box.space.sync
> -s:format({{name = 'id', type = 'unsigned'}, {name = 'value', type = 'unsigned'}})
> -_ = s:create_index('primary', {parts = {'id'}})
> -s:insert{1, 1}
> +_ = box.space.sync:create_index('pk')
> +box.space.sync:insert{1}

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Cyrill Gorcunov]

On Sun, Dec 20, 2020 at 06:01:02PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > Took both notes from some existing example. Would the following be better?
> 
> Yes, thanks, it does not raise any questions now. Although
> you could keep 's' variable to access the space methods shorter.

Will do then, in v5 of series. Thanks a huge for all the feedback, Vlad!

Re: [Tarantool-patches] [PATCH v4 3/3] test: add replication/gh-5446-qsync-eval-quorum.test.lua

[Cyrill Gorcunov]

On Sun, Dec 20, 2020 at 06:01:02PM +0100, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > Took both notes from some existing example. Would the following be better?
> 
> Yes, thanks, it does not raise any questions now. Although
> you could keep 's' variable to access the space methods shorter.

Vlad, here is an interdiff for v4. I addressed all your comments I hope.
If you prefer I'll resend the whole series anew.
---
issue https://github.com/tarantool/tarantool/issues/5446
branch gorcunov/gh-5446-eval-quorum-5

Cyrill Gorcunov (4):
  cfg: add cfg_isnumber helper
  cfg: rework box_check_replication_synchro_quorum
  cfg: support symbolic evaluation of replication_synchro_quorum
  test: add replication/gh-5446-qsync-eval-quorum.test.lua

base-commit: 28f3b2f1e845aff49048d92f9062a4dfa365bf57
--
git diff gorcunov/gh-5446-eval-quorum-4
(I removed interdiff for test code)
--
diff --git a/src/box/box.cc b/src/box/box.cc
index ff5c27743..d3ec1faf3 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -585,7 +585,6 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 		"end\n"
 		"return math.floor(res)\n";
 	const char *expr = cfg_gets("replication_synchro_quorum");
-	int quorum = -1;
 
 	/*
 	 * cfg_gets uses static buffer as well so we need a local
@@ -610,6 +609,7 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 		return -1;
 	}
 
+	int quorum = -1;
 	if (lua_isnumber(tarantool_L, -1))
 		quorum = (int)lua_tonumber(tarantool_L, -1);
 	lua_pop(tarantool_L, 1);
@@ -618,9 +618,6 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 	 * At least we should have 1 node to sync, the weird
 	 * formulas such as N-2 do not guarantee quorums thus
 	 * return an error.
-	 *
-	 * Since diag_set doesn't allow to show the valid range
-	 * lets print a warning too.
 	 */
 	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
 		const char *msg =
@@ -640,8 +637,6 @@ box_eval_replication_synchro_quorum(int nr_replicas)
 static int
 box_check_replication_synchro_quorum(void)
 {
-	int quorum = 0;
-
 	if (!cfg_isnumber("replication_synchro_quorum")) {
 		/*
 		 * The formula uses symbolic name 'N' as
@@ -656,27 +651,20 @@ box_check_replication_synchro_quorum(void)
 		 * sure that the cycle won't take too much time.
 		 */
 		for (int i = 1; i < VCLOCK_MAX; i++) {
-			quorum = box_eval_replication_synchro_quorum(i);
-			if (quorum < 0)
+			if (box_eval_replication_synchro_quorum(i) < 0)
 				return -1;
 		}
-		/*
-		 * Just to make clear the number we return here doesn't
-		 * have any special meaning, only errors are matter.
-		 * The real value is dynamic and will be updated on demand.
-		 */
 		return 0;
-	} else {
-		quorum = cfg_geti("replication_synchro_quorum");
 	}
 
+	int quorum = cfg_geti("replication_synchro_quorum");
 	if (quorum <= 0 || quorum >= VCLOCK_MAX) {
 		diag_set(ClientError, ER_CFG, "replication_synchro_quorum",
 			 "the value must be greater than zero and less than "
 			 "maximal number of replicas");
 		return -1;
 	}
-	return quorum;
+	return 0;
 }
 
 static double
@@ -877,7 +865,7 @@ box_check_config(void)
 	box_check_replication_connect_timeout();
 	box_check_replication_connect_quorum();
 	box_check_replication_sync_lag();
-	if (box_check_replication_synchro_quorum() < 0)
+	if (box_check_replication_synchro_quorum() != 0)
 		diag_raise();
 	if (box_check_replication_synchro_timeout() < 0)
 		diag_raise();
@@ -1022,10 +1010,6 @@ box_set_replication_sync_lag(void)
 	replication_sync_lag = box_check_replication_sync_lag();
 }
 
-/**
- * Renew replication_synchro_quorum value if defined
- * as a formula and we need to recalculate it.
- */
 void
 box_update_replication_synchro_quorum(void)
 {
@@ -1057,8 +1041,7 @@ box_update_replication_synchro_quorum(void)
 int
 box_set_replication_synchro_quorum(void)
 {
-	int value = box_check_replication_synchro_quorum();
-	if (value < 0)
+	if (box_check_replication_synchro_quorum() != 0)
 		return -1;
 	box_update_replication_synchro_quorum();
 	return 0;
diff --git a/src/box/box.h b/src/box/box.h
index c3e1a1276..8a7cda194 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -251,8 +251,8 @@ void box_set_replication_timeout(void);
 void box_set_replication_connect_timeout(void);
 void box_set_replication_connect_quorum(void);
 void box_set_replication_sync_lag(void);
-int box_set_replication_synchro_quorum(void);
 void box_update_replication_synchro_quorum(void);
+int box_set_replication_synchro_quorum(void);
 int box_set_replication_synchro_timeout(void);
 void box_set_replication_sync_timeout(void);
 void box_set_replication_skip_conflict(void);
-- 
2.26.2