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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.