[Tarantool-patches] [PATCH v3 2/2] memtx: allow quota overuse for truncation and deletion

[Vladislav Shpilevoy]

Thanks for the patch!

I answer some of Nikita's questions below and
provide my own review further.

See 7 comments below.

On 21/01/2020 12:42, Nikita Pettik wrote:
> On 20 Jan 21:13, Ilya Kosarev wrote:
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 1b2b27d61..678c9ffe3 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space)
>>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>>  	assert(ops_buf_end < buf + buf_size);
>>  
>> +	struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
>> +	memtx_engine_set_quota_strictness(truncate_space->engine, false);
> 
> I wouldn't call it strictness. In fact you simply turn quota on/off.
> So I'd better call it quota_enable/disable.
> 
> Nit: please accompany your code with (at least brief) comments. 

1. On/off sounds good.

>> +
>>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>>  		       ops_buf, ops_buf_end, 0, NULL) != 0)
>>  		diag_raise();
> 
> If box_upsert() fails, quota is not re-enabled, isn't it?

2. This is a bug, yes. But not the only one. The upsert will write
to WAL and yield. During that yield the quota is turned off. And
any other fiber can interfere and overuse the quota even more with
something not related to delete/truncate.

This can be fixed, for example, by making txn_begin() + quota_off() +
upsert() + quota_on() + commit(). Also we could just not touch truncate
and fix delete() only. Delete() can never leave the quota overused,
because is not stored anywhere in memtx. While truncate is stored in
_truncate. And delete would be sufficient, because can be used to free
the quota enough so as to call truncate()/whatever else the user wanted.

>> +
>> +	memtx_engine_set_quota_strictness(truncate_space->engine, true);
>>  }
>>  
>>  int
>> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
>> index 4da80824a..c19205d34 100644
>> --- a/src/box/memtx_engine.c
>> +++ b/src/box/memtx_engine.c
>> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size)
>>  	return 0;
>>  }
>>  
>> +void
>> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict)
>> +{
>> +	if (strncmp(engine->name, "memtx", 5) != 0)
>> +		return;
> 
> memtx_ prefix in the name of this function assumes that it is called
> only for memtx engine. Note that there's no such checks in memtx_engine
> interface functions.
> 
>> +	struct memtx_engine *memtx = (struct memtx_engine *)engine;
>> +	quota_set_strictness(&memtx->quota, strict);
>  
> What happens if quota is turned on back, but it still exceeds limit?
> I guess it would turn out to be in inconsistent state, since in
> normal operation mode there's no chance of being beyond the limit.

3. That problem is not solvable. There is always a chance, that truncate
won't free any memory (for example, the target space could be empty
already). If, after the quota is back on, it appears to be overused, nothing
can be done with it. It will stay overused until more delete/truncate will be
executed and actually delete something, or the user will increase memtx memory.

> commit 45d9f491f91d93c8835ca725c234f7113c0d2aa8
> Author: Ilya Kosarev <i.kosarev at tarantool.org>
> Date:   Mon Jan 20 20:21:37 2020 +0300
> 
>     memtx: allow quota overuse for truncation and deletion
>     
>     
>     Trying to perform space:truncate() and space:delete() while reaching
>     memtx_memory limit we could experience slab allocator failure. This
>     behavior seems to be quite surprising for users. Now we are allowing
>     to overuse memtx quota if needed for truncation or deletion, using flag
>     in struct quota. After performing truncation or deletion we reset the
>     flag so that quota can be overused any more.
>     
>     Closes #3807
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1b2b27d61..678c9ffe3 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1321,9 +1321,14 @@ space_truncate(struct space *space)
>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>  	assert(ops_buf_end < buf + buf_size);
>  
> +	struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
> +	memtx_engine_set_quota_strictness(truncate_space->engine, false);
> +
>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>  		       ops_buf, ops_buf_end, 0, NULL) != 0)
>  		diag_raise();
> +
> +	memtx_engine_set_quota_strictness(truncate_space->engine, true);
>  }

4. IMO, we should not touch quota at all until we see a not empty diag
with OOM error type in it. Both here and in delete. Truncate is not
a frequent operation (although it may be in some rare installations),
but delete surely should not touch quota each time.

>  int
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 4da80824a..c19205d34 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1090,6 +1090,15 @@ memtx_engine_set_memory(struct memtx_engine *memtx, size_t size)
>  	return 0;
>  }
>  
> +void
> +memtx_engine_set_quota_strictness(struct engine *engine, bool strict)
> +{
> +	if (strncmp(engine->name, "memtx", 5) != 0)
> +		return;

5. That check should not exist. It slows down the caller, and
in upper code you always know whether you work with memtx or
not. So it makes no sense to check it here. Truncate is
always about insertion into a memtx space. Delete touches qouta
inside memtx_space implementation. No chances that a vinyl space
would call this function.

> +	struct memtx_engine *memtx = (struct memtx_engine *)engine;
> +	quota_set_strictness(&memtx->quota, strict);
> +}
> +
>  void
>  memtx_engine_set_max_tuple_size(struct memtx_engine *memtx, size_t max_size)
>  {
> diff --git a/src/lib/small b/src/lib/small
> index 50cb78743..bdcd569f9 160000
> --- a/src/lib/small
> +++ b/src/lib/small
> @@ -1 +1 @@
> -Subproject commit 50cb7874380b286f3b34c82299cf5eb88b0d0059
> +Subproject commit bdcd569f9ed753efb805f708089ca86e2520a44f

6. Please, write tests. They should include the bug test itself, and
complex things such as quota overuse kept in case truncate() didn't
free anything. You can use memtx_memory parameter to make it small
enough so as the test would not take much time.

7. Seems like you changed something in small/. You need to send it as
a separate patch with its own tests, and all.