From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7DEFE469710 for ; Fri, 20 Nov 2020 14:34:57 +0300 (MSK) Received: by mail-lf1-f44.google.com with SMTP id 74so12985641lfo.5 for ; Fri, 20 Nov 2020 03:34:57 -0800 (PST) Date: Fri, 20 Nov 2020 14:34:54 +0300 From: Cyrill Gorcunov Message-ID: <20201120113454.GB875895@grain> References: <20201119194100.840495-1-gorcunov@gmail.com> <20201119194100.840495-4-gorcunov@gmail.com> <1a8fa5cf-ad2a-26f3-3a9d-95367c014cbf@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1a8fa5cf-ad2a-26f3-3a9d-95367c014cbf@tarantool.org> 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: Serge Petrenko Cc: tml , Vladislav Shpilevoy 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. > 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)) > 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. 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) - 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. > I.e. not lua calls  box_cfg_set_..., but box_cfg() itself does. But only on > the first box.cfg call.