Re: [Tarantool-patches] [PATCH v4 3/4] memtx: allow quota overuse for truncation and deletion

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

Thanks for the patch!

See 7 comments below.

On 14/02/2020 20:39, 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 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't be overused any more.
> 
> Closes #3807
> ---
>  src/box/box.cc        | 27 ++++++++++++++++++++++++++-
>  src/box/memtx_space.c | 18 ++++++++++++++----
>  2 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 1b2b27d61..1a63f25a2 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1321,9 +1321,34 @@ space_truncate(struct space *space)
>  	ops_buf_end = mp_encode_uint(ops_buf_end, 1);
>  	assert(ops_buf_end < buf + buf_size);

1. Please, add a comment why do you open a transaction.

> +	struct txn *txn = NULL;
> +	struct txn_savepoint *txn_svp = NULL;
> +	if (!box_txn()) {
> +		txn = txn_begin();
> +		if (txn == NULL)
> +			diag_raise();
> +	} else {
> +		txn_svp = box_txn_savepoint();
> +		if (txn_svp == NULL)
> +			diag_raise();
> +	}
> +	struct space *truncate_space = space_cache_find_xc(BOX_TRUNCATE_ID);
> +	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, false);

2. This line is out of 80 symbols. This is a very simple rule. If a line
is longer than 80 symbols, then it is not correct. Please, customize your
text editor. Every editor has an option to show so called 'rulers' on
certain columns. I set mine to 80, 66, and 50 for commit titles. Then you
will see too long lines before sending a patch. This helps a lot.

3. You repeat this super long thing 3 times:

    &((struct memtx_engine *)truncate_space->engine)->quota

in one function. Wouldn't it be better to save it once into a
'struct quota *' variable and use it?

>  	if (box_upsert(BOX_TRUNCATE_ID, 0, tuple_buf, tuple_buf_end,
> -		       ops_buf, ops_buf_end, 0, NULL) != 0)
> +		       ops_buf, ops_buf_end, 0, NULL) != 0) {
> +		quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);

4. Now when I see quota_on_off() in action I think the name is
really confusing. For example:

    quota_on_off(true)

What does it mean? True means 'on' or 'off'? I propose to make two
functions: quota_enable() and quota_disable(), with one
'struct quota *' argument.

> +		if (txn != NULL)
> +			txn_rollback(txn);
> +		else
> +			box_txn_rollback_to_savepoint(txn_svp);
> +		fiber_gc();
>  		diag_raise();
> +	}
> +	quota_on_off(&((struct memtx_engine *)truncate_space->engine)->quota, true);
> +	if (txn != NULL && txn_commit(txn) != 0) {
> +		fiber_gc();
> +		diag_raise();
> +	}
>  }
>  
>  int
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 6ef84e045..3542450f7 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -28,6 +28,7 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#include "exception.h"
>  #include "memtx_space.h"
>  #include "space.h"
>  #include "iproto_constants.h"
> @@ -354,10 +355,19 @@ memtx_space_execute_delete(struct space *space, struct txn *txn,
>  	struct tuple *old_tuple;
>  	if (index_get(pk, key, part_count, &old_tuple) != 0)
>  		return -1;
> -	if (old_tuple != NULL &&
> -	    memtx_space->replace(space, old_tuple, NULL,
> -				 DUP_REPLACE_OR_INSERT, &stmt->old_tuple) != 0)
> -		return -1;
> +	if (old_tuple != NULL) {
> +		bool oom = !diag_is_empty(diag_get()) &&
> +			   diag_last_error(diag_get())->type == &type_OutOfMemory;

5. Please, rename to 'is_oom'. For flags we always use 'is_/has_/...'
prefix. Except some places, where it was missed at review stage.

6. This line is also out of 80 symbols. And two lines below about
quota enabling/disabling.

7. I am not sure what is happening here. You check for 'oom'
*before* doing actual change? Why?! An oom can happen only below,
in memtx_space->replace().

> +		if (oom)
> +			quota_on_off(&((struct memtx_engine *)space->engine)->quota, false);
> +		int rc = memtx_space->replace(space, old_tuple, NULL,
> +					      DUP_REPLACE_OR_INSERT,
> +					      &stmt->old_tuple);
> +		if (oom)
> +			quota_on_off(&((struct memtx_engine *)space->engine)->quota, true);
> +		if (rc != 0)
> +			return -1;
> +	}
>  	stmt->engine_savepoint = stmt;
>  	*result = stmt->old_tuple;
>  	return 0;