From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 8F7EA45C304 for ; Sun, 13 Dec 2020 21:12:53 +0300 (MSK) References: <20201203140446.66312-1-gorcunov@gmail.com> <20201203140446.66312-3-gorcunov@gmail.com> <33204324-de84-c12d-f22a-51659f0b7d20@tarantool.org> <20201207214825.GF2303@grain> <20201208080230.GG2303@grain> <4091c2f3-9990-80a2-1096-5c195903dac9@tarantool.org> <20201211122558.GF544004@grain> From: Vladislav Shpilevoy Message-ID: Date: Sun, 13 Dec 2020 19:12:51 +0100 MIME-Version: 1.0 In-Reply-To: <20201211122558.GF544004@grain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 2/3] cfg: support 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: Mons Anderson , tml On 11.12.2020 13:25, Cyrill Gorcunov via Tarantool-patches wrote: > On Thu, Dec 10, 2020 at 12:22:28AM +0100, Vladislav Shpilevoy wrote: >>> >>> @TarantoolBot document >>> Title: Synchronous replication >> >> 1. Please, be more specific. Imagine if all github tickets about >> qsync would have the same title 'Synchronous replication'. >> >>> The plain integer value might be convenient for simple scenarios. >> >> The plain integer for what? Please, keep in mind that the docteam >> will see everything below "@TarantoolBot document" mark. It means, >> at this sentence the reader is already lost, because no context at >> all. This text will be read not by developers, and out of the >> commit message context. >> >> Only by replication_synchro_quorum below the reader may assume, >> that as a 'plain integer' you mean the old way of specifying >> replication_synchro_quorum. Which we know, but the docteam does >> not remember, that replication_synchro_quorum was an integer before, >> and now can be a string. >> >> State explicitly what is the request is about, how it worked before, >> what changed now, and why. 'Why' part is good - that it handles the >> 'dynamic' part for you. > > OK, thanks! I suppose the plain integers are allowed for simplicity > mostly, right? No. Mostly because we didn't think of having the option as an expression. Because we never did that. This option is a first dynamically auto-configured one. >>> + int value = -1; >>> + const char *expr = cfg_gets("replication_synchro_quorum"); >>> + const char *buf = tt_sprintf(fmt, expr, nr_replicas); >> >> 3. What is the result is >= TT_STATIC_BUF_LEN? I suspect a user will >> get a surprising error message, or will even get no error, but the >> expression will be just truncated. Does not look good. >> >> Oh, shit. I just found that cfg_gets() also uses the static buffer. >> Besides, it just truncates the string value to 256 chars. So whatever >> you specify as replication_synchro_quorum, if it is longer than 256, >> it is silently truncated. Also does not look good. But don't know >> how to fix it, and if we want to fix it now. > > For now I simply revert back to local stack 1K buffer for formula > evaluation, like it was before. I think 1K would be more than enough > and allows us to detect if trimming happened. No, the problem is not in the local buffer only. The problem is that cfg_gets() does the trim. So you didn't remove the trim. It still happens inside of cfg_gets(). And I assume that is worth fixing somehow. Current cfg module implementation is very naive and easy to break. Perhaps an easy fix would be to make cfg_gets() take a buffer + buffer size parameters, where it would copy the value. And would return something bad if it didn't fit. A more complex solution - make cfg_gets() return an out parameter + string pointer + string size. And make it keep the string on Lua stack. So the string pointer remains valid. Then after you would finish with it, you would call cfg_puts() with the out parameter you got earlier. This approach is 0 copy. But still it is not related to your patch directly, so I am fine with using a stack buffer here. > /* > * 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, > replicaset.registered_count); > if (len >= (int)sizeof(buf)) { > diag_set(ClientError, ER_CFG, > "replication_synchro_quorum", > "the formula is too big"); > return -1; > } > > >>> + /* >>> + * At least we should have 1 node to sync, thus >>> + * if the formula has evaluated to some negative >>> + * value (say it was n-2) do not treat it as an >>> + * error but just yield a minimum valid magnitude. >>> + */ >>> + if (value >= VCLOCK_MAX) { >>> + const int value_max = VCLOCK_MAX - 1; >>> + say_warn("replication_synchro_quorum evaluated " >>> + "to value %d, set to %d", >>> + value, value_max); >>> + value = value_max; >> >> 4. When I said I want to see a warning when the quorum is >> evaluated to 0, while number of replicas is > 0, I didn't mean to >> delete the validation at all. >> >> Your example about 'n-2' is a proof that a negative value means >> an issue in user's code. Because if node count is 3, the quorum >> will be 1, and synchro guarantees simply don't work. > > This is not anyhow different from using plain integers here. You know > I told Mons several times already -- I don't like this "formula" approach > at all. I don't think users gonna be using some complex formulas here > and I don't understand where it might be needed. N-2 is not complex. It is just stupid. But users may want to use N*3/4 or max(N-1, 1), or something like that. N/2 + 1 is a minimal possible value when guarantees start working. Some users may want better guarantees. And you can't cover them all with special hardcoded values. Here I agree with Mons, that being able to calculate the option dynamically is super cool, and potentially may be useful for other options in future. > When one start using synchronious replication the only thing he is interested > in -- data consistency, ie canonical N/2+1 quorum. And that's all. N/2 + 1 is a minimal limit. You may want it bigger in case you want more safety and you are ready to sacrifice some latency for that.