From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 68D7F469710 for ; Fri, 20 Nov 2020 14:56:13 +0300 (MSK) References: <20201119194100.840495-1-gorcunov@gmail.com> <20201119194100.840495-4-gorcunov@gmail.com> <1a8fa5cf-ad2a-26f3-3a9d-95367c014cbf@tarantool.org> <20201120113454.GB875895@grain> From: Serge Petrenko Message-ID: Date: Fri, 20 Nov 2020 14:56:12 +0300 MIME-Version: 1.0 In-Reply-To: <20201120113454.GB875895@grain> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [RFC 3/4] cfg: prepare symbolic evaluation of replication_synchro_quorum List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml , Vladislav Shpilevoy 20.11.2020 14:34, Cyrill Gorcunov пишет: > On Fri, Nov 20, 2020 at 01:32:42PM +0300, Serge Petrenko wrote: >>> +/** >>> + * Evaluate replicaion syncro quorum number from a formula. >>> + */ >>> +int >>> +eval_replication_synchro_quorum(int nr_replicas) >>> +{ >>> + const char fmt[] = >>> + "local f, err = loadstring(\"return (%s)\")\n" >>> + "if not f then return 'failed to load \"%s\"' end\n" >>> + "setfenv(f, { n = %d })\n" >>> + "local ok, res = pcall(f)\n" >>> + "if not ok then return res end\n" >>> + "return math.floor(res)\n"; >>> + char buf[512]; >>> + int value = -1; >>> + >>> + errno = 0; >>> + >>> + const char *expr = cfg_gets("replication_synchro_quorum"); >>> + size_t ret = snprintf(buf, sizeof(buf), fmt, expr, >>> + expr, nr_replicas); >>> + if (ret >= sizeof(buf)) { >>> + errno = EINVAL; >>> + diag_set(ClientError, ER_CFG, >>> + "replication_synchro_quorum", >>> + "the expression is too big"); >>> + return -1; >>> + } >>> + >>> + luaL_loadstring(tarantool_L, buf); >>> + lua_call(tarantool_L, 0, 1); >>> + >>> + if (lua_isnumber(tarantool_L, -1)) { >>> + value = (int)lua_tonumber(tarantool_L, -1); >>> + } else { >>> + assert(lua_isstring(tarantool_L, -1)); >>> + errno = EINVAL; >>> + diag_set(ClientError, ER_CFG, >>> + "replication_synchro_quorum", >>> + lua_tostring(tarantool_L, -1)); >>> + } >>> + lua_pop(tarantool_L, 1); >>> + return value; >>> +} >> Better make  the function return -1 on an error, and when the function >> evaluates to something negative, >> log the eval result and return 1. This way you won't need errno. > We need to distinguish wrong syntax and wrong evaluated values somehow > which means using -1 for anything wont work here. > > - wrong syntax (which includes too long formula): -1 && errno > - on correct syntax we simply return the result the caller has > to check if returned value fits the allowed range. > > Or I'm missing something. I was thinking that we're gonna return something like max(1, min(value, 31)) so that any evaluated number is correct. Lets better discuss this verbally then. > >> You should either check every possible value, from 1 to VCLOCK_MAX - 1, >> to make sure, say, that no division by zero is involved for some input. > That's the good point. Another question if we should allow formulas like > n-2, and while n <= 2 assume the quorum to be 1? Ie max(1, eval(n)) Yes, that's what I was  speaking about above. So that when the formula may be evaluated correctly (i.e. without division by zero or syntax errors) its result will automatically be correct. > >> or check a single value, say, 1, or 2 or whatever, to make sure that the >> expression in replication_synchro_quorum is  at least valid Lua code. >> >> Why check only min and max inputs? > Lets consider the evaluation a bit closer. Denote arguments as x, thus > we will have x ∈ N, where are x's are strictly ordered, thus to get > f(x) < 0 between min/max values we need to use nonlinear f. I must confess > I don't know what is allowed inside Lua evaluator and really don't like > that we're allowing to pass completely arbitrary formula here. Moreover > what would happen if f would be using some weird data so that if will be > linear on bootstrap procedure but then switched to something more complex? > All this arbitrary evaluation looks very very fishy and I bet there will > be problems because this is the data we do not control completely. I guess we shouldn't be this crazy about what is allowed in this formula and what's not. If a user has access to box.cfg{}, he may evaluate any expression he wishes anyway. Anyway, this is subject of a verbal discussion. > > Back to the former question -- initially I assume the f gonna be linear > and eval in min/max will be enough. But of course this is not correct. > > You know I can pass all N's here but still this doesn't guarantee anything :( > That's why I'm for more strict rules here: > > - allow some symbolic names such as > "all" -> (alias for f(x) = n) > "canonical" -> (alias for f(x) = n/2 + 1) Sounds good to me. AFAIR others were agains it, though. > > - implement own syntax analizyer/evaluator which would allow only > basic operation {+,/} over the set of {n} (which will give us > a linear functioni by definition). This of course will require > a way more code so not right now. > >>> @@ -368,7 +368,6 @@ local dynamic_cfg_skip_at_load = { >>> replication_connect_quorum = true, >>> replication_sync_lag = true, >>> replication_sync_timeout = true, >>> - replication_synchro_quorum = true, >> You shouldn't remove replication_synchro_quorum from here. >> This table  lists the options which are set directly from `box_cfg` in >> specific order. > No, this table is to _skip_ evaluation on bootup. But we have to verify > the default value to evaluate. Yes, that's what I'm talking about. Even if the cfg option from  this  list is 'skipped' in  lua, it's referenced directly from box_cfg_xc(). Othervise the `box_cfg_set_...` will be called twice. Once  from box_cfg_xc(), second time from this lua code. To be more verbose, all the setters from dynamic_cfg_skip_at_load are called on bootstrap. But from box_cfg_xc() in C, not from Lua. If you remove an entry from dynamic_cfg_skip_at_load, the corresponding setter will be called twice. > >> I.e. not lua calls  box_cfg_set_..., but box_cfg() itself does. But only on >> the first box.cfg call. -- Serge Petrenko