[Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion

[Tarantool-patches] [PATCH 0/3] Safe allocation on truncation and deletion

[Ilya Kosarev]

space:truncate() and space:delete() could fail on memory allocations
when reaching memtx_memory limit. As far as it is quite an ill
behaviour, it is fixed in this patchset through denial of extra
memory reservation and unconstrained allocation of service tuples
in BOX_TRUNCATE_ID space.

Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-3807-safe-alloc-on-truncation
Issue: https://github.com/tarantool/tarantool/issues/3807

Ilya Kosarev (3):
  b-tree: return NULL on matras_alloc fail
  memtx: don't reserve extra memory if not needed
  tuple: use calloc for service truncation tuples

 src/box/memtx_engine.c   | 50 ++++++++++++++++++++++++++--------------
 src/box/memtx_engine.h   |  2 +-
 src/box/memtx_space.c    | 34 ++++++++++++++++++++-------
 src/box/tuple.c          | 16 +++++++++----
 src/box/tuple.h          |  2 +-
 src/box/tuple_format.c   |  1 +
 src/box/tuple_format.h   | 10 +++++++-
 src/box/vy_stmt.c        |  3 ++-
 src/lib/salad/bps_tree.h |  2 ++
 9 files changed, 87 insertions(+), 33 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail

[Ilya Kosarev]

In bps_tree_create_leaf we use matras_alloc in case
bps_tree_garbage_pop didn't work out. However it also might not
succeed. Then we need to return NULL instead of dereferencing NULL
pointer.

Prerequisites: #3807
---
 src/lib/salad/bps_tree.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
index d28b53f53c8..51f5c8b5556 100644
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -2149,6 +2149,8 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id)
 			       bps_tree_garbage_pop(tree, id);
 	if (!res)
 		res = (struct bps_leaf *)matras_alloc(&tree->matras, id);
+	if (!res)
+		return NULL;
 	res->header.type = BPS_TREE_BT_LEAF;
 	tree->leaf_count++;
 	return res;
-- 
2.17.1

[Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed

[Ilya Kosarev]

memtx_index_extent_reserve was always being called from
memtx_space_replace_all_keys. It could lead to slab allocator failure
while reaching memtx_memory limit, which is especially distressing on
space:truncate() and space:delete() invocations. we  Now it is not
being called if not needed, including space:truncate() and
space:delete() cases.

Part of #3807
---
 src/box/memtx_space.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 6ef84e04564..1e8759778e7 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -260,10 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple,
 	 * Ensure we have enough slack memory to guarantee
 	 * successful statement-level rollback.
 	 */
-	if (memtx_index_extent_reserve(memtx, new_tuple != NULL ?
-				       RESERVE_EXTENTS_BEFORE_REPLACE :
-				       RESERVE_EXTENTS_BEFORE_DELETE) != 0)
+	if (space->index_count > 1 && new_tuple != NULL) {
+		if (memtx_index_extent_reserve(memtx, new_tuple != NULL ?
+					       RESERVE_EXTENTS_BEFORE_REPLACE :
+					       RESERVE_EXTENTS_BEFORE_DELETE) != 0)
+			return -1;
+	}
+	ERROR_INJECT(ERRINJ_INDEX_ALLOC, {
+		/* same error as in mempool_alloc */
+		diag_set(OutOfMemory, MEMTX_EXTENT_SIZE,
+			 "mempool", "new slab");
 		return -1;
+	});
 
 	uint32_t i = 0;
 
-- 
2.17.1

[Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples

[Ilya Kosarev]

Trying to perform space:truncate() while reaching memtx_memory limit we
could experience slab allocator failure on allocation for service tuple
being upserted into service space to keep record of truncations. This
behavior seems to be quite surprising for users. Now we are using
calloc() to allocate tuples in BOX_TRUNCATE_ID space.

Closes: #3807
---
 src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++++--------------
 src/box/memtx_engine.h |  2 +-
 src/box/memtx_space.c  | 20 ++++++++++++-----
 src/box/tuple.c        | 16 ++++++++++----
 src/box/tuple.h        |  2 +-
 src/box/tuple_format.c |  1 +
 src/box/tuple_format.h | 10 ++++++++-
 src/box/vy_stmt.c      |  3 ++-
 8 files changed, 74 insertions(+), 30 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 23ccc4703fe..59148059f2b 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -1113,7 +1113,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx)
 }
 
 struct tuple *
-memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
+memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc)
 {
 	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
 	assert(mp_typeof(*data) == MP_ARRAY);
@@ -1139,15 +1139,27 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
 	}
 
 	struct memtx_tuple *memtx_tuple;
-	while ((memtx_tuple = smalloc(&memtx->alloc, total)) == NULL) {
-		bool stop;
-		memtx_engine_run_gc(memtx, &stop);
-		if (stop)
-			break;
-	}
-	if (memtx_tuple == NULL) {
-		diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple");
-		goto end;
+	if (use_calloc) {
+		memtx_tuple = calloc(1, total);
+		if (memtx_tuple == NULL) {
+			diag_set(OutOfMemory, total, "malloc",
+						     "memtx_tuple");
+			goto end;
+		}
+		format->is_malloced = true;
+	} else {
+		while ((memtx_tuple = smalloc(&memtx->alloc,
+					      total)) == NULL) {
+			bool stop;
+			memtx_engine_run_gc(memtx, &stop);
+			if (stop)
+				break;
+		}
+		if (memtx_tuple == NULL) {
+			diag_set(OutOfMemory, total, "slab allocator",
+						     "memtx_tuple");
+			goto end;
+		}
 	}
 	tuple = &memtx_tuple->base;
 	tuple->refs = 0;
@@ -1177,16 +1189,20 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)
 	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
 	say_debug("%s(%p)", __func__, tuple);
 	assert(tuple->refs == 0);
-	tuple_format_unref(format);
 	struct memtx_tuple *memtx_tuple =
 		container_of(tuple, struct memtx_tuple, base);
 	size_t total = tuple_size(tuple) + offsetof(struct memtx_tuple, base);
-	if (memtx->alloc.free_mode != SMALL_DELAYED_FREE ||
-	    memtx_tuple->version == memtx->snapshot_version ||
-	    format->is_temporary)
-		smfree(&memtx->alloc, memtx_tuple, total);
-	else
-		smfree_delayed(&memtx->alloc, memtx_tuple, total);
+	if (format->is_malloced) {
+		free(memtx_tuple);
+	} else {
+		if (memtx->alloc.free_mode != SMALL_DELAYED_FREE ||
+		    memtx_tuple->version == memtx->snapshot_version ||
+		    format->is_temporary)
+			smfree(&memtx->alloc, memtx_tuple, total);
+		else
+			smfree_delayed(&memtx->alloc, memtx_tuple, total);
+	}
+	tuple_format_unref(format);
 }
 
 void
diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h
index f562c66df4f..a2ae8134d30 100644
--- a/src/box/memtx_engine.h
+++ b/src/box/memtx_engine.h
@@ -235,7 +235,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx);
 
 /** Allocate a memtx tuple. @sa tuple_new(). */
 struct tuple *
-memtx_tuple_new(struct tuple_format *format, const char *data, const char *end);
+memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc);
 
 /** Free a memtx tuple. @sa tuple_delete(). */
 void
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 1e8759778e7..c413fd9b1e3 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -332,7 +332,9 @@ memtx_space_execute_replace(struct space *space, struct txn *txn,
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	enum dup_replace_mode mode = dup_replace_mode(request->type);
 	stmt->new_tuple = memtx_tuple_new(space->format, request->tuple,
-					  request->tuple_end);
+					  request->tuple_end,
+					  request->space_id
+					  == BOX_TRUNCATE_ID);
 	if (stmt->new_tuple == NULL)
 		return -1;
 	tuple_ref(stmt->new_tuple);
@@ -406,7 +408,9 @@ memtx_space_execute_update(struct space *space, struct txn *txn,
 		return -1;
 
 	stmt->new_tuple = memtx_tuple_new(format, new_data,
-					  new_data + new_size);
+					  new_data + new_size,
+					  request->space_id
+					  == BOX_TRUNCATE_ID);
 	if (stmt->new_tuple == NULL)
 		return -1;
 	tuple_ref(stmt->new_tuple);
@@ -475,7 +479,9 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn,
 			return -1;
 		}
 		stmt->new_tuple = memtx_tuple_new(format, request->tuple,
-						  request->tuple_end);
+						  request->tuple_end,
+						  request->space_id
+						  == BOX_TRUNCATE_ID);
 		if (stmt->new_tuple == NULL)
 			return -1;
 		tuple_ref(stmt->new_tuple);
@@ -499,7 +505,9 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn,
 			return -1;
 
 		stmt->new_tuple = memtx_tuple_new(format, new_data,
-						  new_data + new_size);
+						  new_data + new_size,
+						  request->space_id
+						  == BOX_TRUNCATE_ID);
 		if (stmt->new_tuple == NULL)
 			return -1;
 		tuple_ref(stmt->new_tuple);
@@ -548,7 +556,9 @@ memtx_space_ephemeral_replace(struct space *space, const char *tuple,
 {
 	struct memtx_space *memtx_space = (struct memtx_space *)space;
 	struct tuple *new_tuple = memtx_tuple_new(space->format, tuple,
-						  tuple_end);
+						  tuple_end,
+						  space->def->id
+						  == BOX_TRUNCATE_ID);
 	if (new_tuple == NULL)
 		return -1;
 	struct tuple *old_tuple;
diff --git a/src/box/tuple.c b/src/box/tuple.c
index 4d676b090c2..bafa8e0cb5c 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -60,7 +60,7 @@ static void
 runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple);
 
 static struct tuple *
-runtime_tuple_new(struct tuple_format *format, const char *data, const char *end);
+runtime_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc);
 
 /** A virtual method table for tuple_format_runtime */
 static struct tuple_format_vtab tuple_format_runtime_vtab = {
@@ -71,7 +71,7 @@ static struct tuple_format_vtab tuple_format_runtime_vtab = {
 };
 
 static struct tuple *
-runtime_tuple_new(struct tuple_format *format, const char *data, const char *end)
+runtime_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc)
 {
 	assert(format->vtab.tuple_delete == tuple_format_runtime_vtab.tuple_delete);
 
@@ -86,8 +86,13 @@ runtime_tuple_new(struct tuple_format *format, const char *data, const char *end
 
 	size_t data_len = end - data;
 	size_t total = sizeof(struct tuple) + field_map_size + data_len;
-	tuple = (struct tuple *) smalloc(&runtime_alloc, total);
+	if (use_calloc) {
+		tuple = (struct tuple *) calloc(1, total);
+		format->is_malloced = true;
+	} else
+		tuple = (struct tuple *) smalloc(&runtime_alloc, total);
 	if (tuple == NULL) {
+		format->is_malloced = false;
 		diag_set(OutOfMemory, (unsigned) total,
 			 "malloc", "tuple");
 		goto end;
@@ -114,8 +119,11 @@ runtime_tuple_delete(struct tuple_format *format, struct tuple *tuple)
 	say_debug("%s(%p)", __func__, tuple);
 	assert(tuple->refs == 0);
 	size_t total = tuple_size(tuple);
+	if (format->is_malloced)
+		free(tuple);
+	else
+		smfree(&runtime_alloc, tuple, total);
 	tuple_format_unref(format);
-	smfree(&runtime_alloc, tuple, total);
 }
 
 int
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 9a887721986..7fa62659acc 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -431,7 +431,7 @@ tuple_format(struct tuple *tuple)
 static inline struct tuple *
 tuple_new(struct tuple_format *format, const char *data, const char *end)
 {
-	return format->vtab.tuple_new(format, data, end);
+	return format->vtab.tuple_new(format, data, end, false);
 }
 
 /**
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 312c96621fb..d0e60acd334 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -789,6 +789,7 @@ tuple_format_new(struct tuple_format_vtab *vtab, void *engine,
 		memset(&format->vtab, 0, sizeof(format->vtab));
 	format->engine = engine;
 	format->is_temporary = is_temporary;
+	format->is_malloced = false;
 	format->is_ephemeral = is_ephemeral;
 	format->exact_field_count = exact_field_count;
 	format->epoch = ++formats_epoch;
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index 021072d3d4c..cb9f0a0407d 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -82,7 +82,7 @@ struct tuple_format_vtab {
 	 */
 	struct tuple*
 	(*tuple_new)(struct tuple_format *format, const char *data,
-	             const char *end);
+		     const char *end, bool use_calloc);
 	/**
 	 * Free a tuple_chunk allocated for given tuple and
 	 * data.
@@ -188,6 +188,14 @@ struct tuple_format {
 	 * in progress.
 	 */
 	bool is_temporary;
+	/**
+	 * Tuple is being malloced if we use calloc instead of
+	 * traditional smalloc to allocate it. It is happening
+	 * if the tuple is being upserted into BOX_TRUNCATE_ID
+	 * space as far as we want to perform space:truncate()
+	 * even if memtx_memory limit is reached.
+	 */
+	bool is_malloced;
 	/**
 	 * This format belongs to ephemeral space and thus might
 	 * be shared with other ephemeral spaces.
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index 518a24f43d7..b6af0b00e5d 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -82,8 +82,9 @@ vy_stmt_persistent_flags(struct tuple *stmt, bool is_primary)
 }
 
 static struct tuple *
-vy_tuple_new(struct tuple_format *format, const char *data, const char *end)
+vy_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc)
 {
+	(void)use_calloc;
 	if (tuple_validate_raw(format, data) != 0)
 		return NULL;
 
-- 
2.17.1

Re: [Tarantool-patches] [PATCH 3/3] tuple: use calloc for service truncation tuples

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 13/12/2019 08:05, Ilya Kosarev wrote:
> Trying to perform space:truncate() while reaching memtx_memory limit we
> could experience slab allocator failure on allocation for service tuple
> being upserted into service space to keep record of truncations. This
> behavior seems to be quite surprising for users. Now we are using
> calloc() to allocate tuples in BOX_TRUNCATE_ID space.
> 
> Closes: #3807
> ---
>  src/box/memtx_engine.c | 50 ++++++++++++++++++++++++++++--------------
>  src/box/memtx_engine.h |  2 +-
>  src/box/memtx_space.c  | 20 ++++++++++++-----
>  src/box/tuple.c        | 16 ++++++++++----
>  src/box/tuple.h        |  2 +-
>  src/box/tuple_format.c |  1 +
>  src/box/tuple_format.h | 10 ++++++++-
>  src/box/vy_stmt.c      |  3 ++-
>  8 files changed, 74 insertions(+), 30 deletions(-)
> 
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index 23ccc4703fe..59148059f2b 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -1113,7 +1113,7 @@ memtx_leave_delayed_free_mode(struct memtx_engine *memtx)
>  }
>  
>  struct tuple *
> -memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
> +memtx_tuple_new(struct tuple_format *format, const char *data, const char *end, bool use_calloc)

1. Oh. No. Please. The whole point of format vtable is to avoid that.

>  {
>  	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
>  	assert(mp_typeof(*data) == MP_ARRAY);
> @@ -1139,15 +1139,27 @@ memtx_tuple_new(struct tuple_format *format, const char *data, const char *end)
>  	}
>  
>  	struct memtx_tuple *memtx_tuple;
> -	while ((memtx_tuple = smalloc(&memtx->alloc, total)) == NULL) {
> -		bool stop;
> -		memtx_engine_run_gc(memtx, &stop);
> -		if (stop)
> -			break;
> -	}
> -	if (memtx_tuple == NULL) {
> -		diag_set(OutOfMemory, total, "slab allocator", "memtx_tuple");
> -		goto end;
> +	if (use_calloc) {
> +		memtx_tuple = calloc(1, total);
> +		if (memtx_tuple == NULL) {
> +			diag_set(OutOfMemory, total, "malloc",
> +						     "memtx_tuple");
> +			goto end;
> +		}
> +		format->is_malloced = true;

2. Sorry, we can't change format here. The only reason why format
is not const in that function is because we need increment its
reference counter. A tuple can't change its own format. Especially
in such a hot path.

> +	} else {
> +		while ((memtx_tuple = smalloc(&memtx->alloc,
> +					      total)) == NULL) {
> +			bool stop;
> +			memtx_engine_run_gc(memtx, &stop);
> +			if (stop)
> +				break;
> +		}
> +		if (memtx_tuple == NULL) {
> +			diag_set(OutOfMemory, total, "slab allocator",
> +						     "memtx_tuple");
> +			goto end;
> +		}
>  	}
>  	tuple = &memtx_tuple->base;
>  	tuple->refs = 0;
> @@ -1177,16 +1189,20 @@ memtx_tuple_delete(struct tuple_format *format, struct tuple *tuple)
>  	struct memtx_engine *memtx = (struct memtx_engine *)format->engine;
>  	say_debug("%s(%p)", __func__, tuple);
>  	assert(tuple->refs == 0);
> -	tuple_format_unref(format);
>  	struct memtx_tuple *memtx_tuple =
>  		container_of(tuple, struct memtx_tuple, base);
>  	size_t total = tuple_size(tuple) + offsetof(struct memtx_tuple, base);
> -	if (memtx->alloc.free_mode != SMALL_DELAYED_FREE ||
> -	    memtx_tuple->version == memtx->snapshot_version ||
> -	    format->is_temporary)
> -		smfree(&memtx->alloc, memtx_tuple, total);
> -	else
> -		smfree_delayed(&memtx->alloc, memtx_tuple, total);
> +	if (format->is_malloced) {
> +		free(memtx_tuple);

3. There is a certain purpose for these free_mode, version, blah
blah. These attributes create a read view of the whole memtx engine
to make a snapshot, but do not stop new requests. Tuples are not
deleted during snapshot.

Having ignored the free mode you get this:

====================================================================

    box.cfg{}
    errinj = box.error.injection
    fiber = require('fiber')

    s = box.schema.create_space('test')
    pk = s:create_index('pk')
    s:truncate()
    s:replace{1}
    box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', true)
    f = fiber.create(box.snapshot)
    box.space._truncate:delete{s.id}

-- The code below improves reproducibility. It creates and
-- deletes several tuples to unref the blessed tuple
-- (tuple_bless), rewrite some recently freed heap chunks.

    s:get{1}
    collectgarbage('collect')
    s:replace{1}
    s:get{1}
    collectgarbage('collect')
    s:replace{1}
    s:get{1}
    collectgarbage('collect')
    s:replace{1}
    s:get{1}
    collectgarbage('collect')

--

    box.error.injection.set('ERRINJ_SNAP_WRITE_DELAY', false)

    tarantool> Process 94390 stopped
    * thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000)
        frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41
    libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell:
    ->  0x7fff5a964ce9 <+41>: rep    movsb  (%rsi), %es:(%rdi)
        0x7fff5a964ceb <+43>: popq   %rbp
        0x7fff5a964cec <+44>: retq   
        0x7fff5a964ced <+45>: cmpq   %rdi, %rsi
    Target 0: (tarantool) stopped.
    (lldb) bt
    * thread #2, name = 'snapshot', stop reason = EXC_BAD_ACCESS (code=2, address=0x105481000)
      * frame #0: 0x00007fff5a964ce9 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 41
        frame #1: 0x00000001003f56ba tarantool`obuf_dup(buf=0x000000010f0ff8b0, data=0x0000000105157aa6, size=85071264) at obuf.c:161:2
        frame #2: 0x00000001003634e8 tarantool`xlog_write_row(log=0x000000010f0ff208, packet=0x000000010f0ff0d8) at xlog.c:1320:7
        frame #3: 0x0000000100038aef tarantool`checkpoint_write_row(l=0x000000010f0ff208, row=0x000000010f0ff0d8) at memtx_engine.c:431:20
        frame #4: 0x0000000100038a2e tarantool`checkpoint_write_tuple(l=0x000000010f0ff208, space_id=330, group_id=0, data="", size=85071264) at memtx_engine.c:459:9
        frame #5: 0x0000000100038811 tarantool`checkpoint_f(ap=0x000000010f000278) at memtx_engine.c:603:8
        frame #6: 0x0000000100005e5a tarantool`fiber_cxx_invoke(f=(tarantool`checkpoint_f at memtx_engine.c:577), ap=0x000000010f000278)(__va_list_tag*), __va_list_tag*) at fiber.h:762:10
        frame #7: 0x000000010018168b tarantool`fiber_loop(data=0x0000000000000000) at fiber.c:830:18
        frame #8: 0x00000001004165e7 tarantool`coro_init at coro.c:110:3

====================================================================

Snapshoting means that you can't solve the issue on the format
level, or tuple level. You need to patch the engine level (IMO).

Honestly, I thought about this issue for quite a long time already,
and I don't have a plan how to fix it. All ways look bad. All of them
assume special patching of _truncate space insertion and crutches for
deletion. For me the most promising way was to introduce an analogue
of vy_quota_force_use() somehow, for _truncate, and use it in a
before_replace trigger of _truncate. Forced use of quota would allow
to temporary overflow the memory limit. But it won't fix deletion.
Maybe the key is in introduction of quota overuse for deletions. I
don't know.

Personally, I don't think the issue is worth fixing at all. If a
user wants to delete/truncate, he can increase memtx memory, do
cleanup, and decrease it back.

Re: [Tarantool-patches] [PATCH 2/3] memtx: don't reserve extra memory if not needed

[Vladislav Shpilevoy]

Thanks for the patch!

On 13/12/2019 08:05, Ilya Kosarev wrote:
> memtx_index_extent_reserve was always being called from
> memtx_space_replace_all_keys. It could lead to slab allocator failure
> while reaching memtx_memory limit, which is especially distressing on
> space:truncate() and space:delete() invocations. we  Now it is not
> being called if not needed, including space:truncate() and
> space:delete() cases.

The thing is that delete also can cause tree rebalancing. Maybe
that is why reservation was done for all cases.

> 
> Part of #3807
> ---
>  src/box/memtx_space.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 6ef84e04564..1e8759778e7 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -260,10 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple,
>  	 * Ensure we have enough slack memory to guarantee
>  	 * successful statement-level rollback.
>  	 */
> -	if (memtx_index_extent_reserve(memtx, new_tuple != NULL ?
> -				       RESERVE_EXTENTS_BEFORE_REPLACE :
> -				       RESERVE_EXTENTS_BEFORE_DELETE) != 0)
> +	if (space->index_count > 1 && new_tuple != NULL) {
> +		if (memtx_index_extent_reserve(memtx, new_tuple != NULL ?
> +					       RESERVE_EXTENTS_BEFORE_REPLACE :
> +					       RESERVE_EXTENTS_BEFORE_DELETE) != 0)
> +			return -1;

Why does it work only when index count > 1? Why delete don't need
a reservation (I pointed out above that it needs)? And why do you
check for new_tuple != NULL second time after having it already
checked in the if's condition?

> +	}
> +	ERROR_INJECT(ERRINJ_INDEX_ALLOC, {
> +		/* same error as in mempool_alloc */
> +		diag_set(OutOfMemory, MEMTX_EXTENT_SIZE,
> +			 "mempool", "new slab");
>  		return -1;
> +	});
>  
>  	uint32_t i = 0;
>  
> 

Re: [Tarantool-patches] [PATCH 1/3] b-tree: return NULL on matras_alloc fail

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 13/12/2019 08:05, Ilya Kosarev wrote:
> In bps_tree_create_leaf we use matras_alloc in case
> bps_tree_garbage_pop didn't work out. However it also might not
> succeed. Then we need to return NULL instead of dereferencing NULL
> pointer.
> 
> Prerequisites: #3807
> ---
>  src/lib/salad/bps_tree.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
> index d28b53f53c8..51f5c8b5556 100644
> --- a/src/lib/salad/bps_tree.h
> +++ b/src/lib/salad/bps_tree.h
> @@ -2149,6 +2149,8 @@ bps_tree_create_leaf(struct bps_tree *tree, bps_tree_block_id_t *id)
>  			       bps_tree_garbage_pop(tree, id);
>  	if (!res)
>  		res = (struct bps_leaf *)matras_alloc(&tree->matras, id);
> +	if (!res)
> +		return NULL;

In case res was not NULL (what is 99.(9)% of all cases), you
will double check it. Please, move the second check into first
'if's body after matras_alloc.