[Tarantool-patches] [PATCH v2 2/2] memtx: increase the memory quota if needed to truncate or delete

[Vladislav Shpilevoy]

Thanks for the patch!

JFI, I am still against this patch. It adds huge and
unnecessary complexity to the code, which we will need
to support forever. It is just not worth the pros the
patch gives.

On 13/01/2020 22:31, Ilya Kosarev wrote:
> 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 increasing
> memtx quota if needed for truncation or deletion. After performing it
> quota is being set back to the previous value if possible, while it
> should be so for almost any case, since we are meant to free some space
> during deletion or truncation.
> 
> Closes #3807
> ---
>  src/box/blackhole.c      |  1 +
>  src/box/box.cc           | 36 +++++++++++++++++++++++++++++++++++-
>  src/box/engine.c         | 11 +++++++++++
>  src/box/engine.h         |  9 +++++++++
>  src/box/memtx_engine.c   | 20 ++++++++++++++++++++
>  src/box/memtx_engine.h   |  4 ++++
>  src/box/service_engine.c |  1 +
>  src/box/sysview.c        |  1 +
>  src/box/vinyl.c          |  1 +
>  9 files changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/blackhole.c b/src/box/blackhole.c
> index 69f1deba1..af587f434 100644
> --- a/src/box/blackhole.c
> +++ b/src/box/blackhole.c
> @@ -194,6 +194,7 @@ static const struct engine_vtab blackhole_engine_vtab = {
>  	/* .commit_checkpoint = */ generic_engine_commit_checkpoint,
>  	/* .abort_checkpoint = */ generic_engine_abort_checkpoint,
>  	/* .collect_garbage = */ generic_engine_collect_garbage,
> +	/* .guarantee_memory = */ generic_engine_guarantee_memory,

The only problem is with memtx engine, and I propose to solve it
on memtx engine level. Vinyl will never need this method.

(But even better I propose to drop the patch and close the issue as
won't fix.)

>  	/* .backup = */ generic_engine_backup,
>  	/* .memory_stat = */ generic_engine_memory_stat,
>  	/* .reset_stat = */ generic_engine_reset_stat,
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1b2b27d61..18c09ce1b 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1321,9 +1341,23 @@ space_truncate(struct space *space)
>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>  	assert(ops_buf_end < buf + buf_size);
>  
> +	size_t total;
> +	bool extended = false;
> +	space->engine->vtab->guarantee_memory(space->engine,
> +					      MEMTX_SLAB_SIZE,
> +					      &total, &extended);
> +

Truncate is always about insertion into the memtx space _truncate.
Here you are calling 'guarantee_memory' for the user space's engine.
And it just won't work in case I try to truncate a vinyl space.

Moreover, the encapsulation of 'memory guarantee' is broken anyway,
because 1) you pass 'MEMTX_SLAB_SIZE' parameter to the engine's
virtual method, 2) below you touch memtx engine explicitly.

>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
>  		       ops_buf, ops_buf_end, 0, NULL) != 0)
>  		diag_raise();
> +
> +	if (extended) {
> +		struct memtx_engine *memtx =
> +			(struct memtx_engine *)space->engine;

Why is space->engine assumed to be memtx? This is a user's space.
It can be vinyl.

> +		size_t new_total = quota_set(&memtx->quota, total);
> +		if (new_total > total)
> +			quota_set(&memtx->quota, quota_used(&memtx->quota));
> +	}
>  }

Since this is a bug fix, there should be a regression test.

But I once again say, that it is even better to drop the patch and
close the issue. This is just sugar, which besides may lead to a side
effect when quota is increased, but can't be decreased back.

I can't find a way how to fix it gracefully and simple.

Especially for truncate which is a total disaster. The space, you have
truncated, could be empty, but you will insert a new tuple into _truncate,
and it will be kept, and will occupy memory. No memory is freed. And you
won't be able to decrease the quota.

AFAIR, at this moment there is no necessity in having _truncate space.
There was something about vylog, why we added that space, and from what
I remember, that problem has already gone. We could just drop _truncate.
Although its existence also may be related to replication. It should be
checked.

Below is an idea I had recently when was thinking about the issue. I don't
think it is good and finished, but maybe it will help you to evolve it to
something better. We could allow to overuse quota. For example, add a flag
to struct quota like 'is_soft'. It is false by default. When quota is not
soft, it works just like now. When we want to allocate something above the
quota, we set the flag to true. Now the quota is soft, and any alloc
succeeds (unless there is no memory in the system, of course).

We set the flag in memtx_space_execute_delete() before
memtx_space->replace(space, old_tuple, NULL), and unset right after it.

For truncate we set this flag in space_truncate() before box_upsert(), and
unset right afterwards (just like you did with guarantee_memory()).

That allows us to overuse quota, but don't touch quota limit. In case the
quota is overused, and is not soft, it behaves just like when it has
reached the limit - does not allow new allocations.