Re: [Tarantool-patches] [PATCH v4 2/4] b-tree: return NULL on matras_alloc fail

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

Thanks for the patch!

See 3 comments below.

On 14/02/2020 20:39, Ilya Kosarev wrote:
> In bps_tree_create_leaf we use matras_alloc in case

1. This is not only about bps_tree_create_leaf() anymore. You
also patched bps_tree_create_inner().

> 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.
> 
> Part of #3807
> ---
>  src/lib/salad/bps_tree.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/salad/bps_tree.h b/src/lib/salad/bps_tree.h
> index d28b53f53..d71340143 100644
> --- a/src/lib/salad/bps_tree.h
> +++ b/src/lib/salad/bps_tree.h
> @@ -3710,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
>  		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
>  		struct bps_inner *new_root = bps_tree_create_inner(tree,
>  				&new_root_id);
> +		if (!new_root)
> +			return -1;

2. Here are two problems.

1) This can't fail, because above there is bps_tree_reserve_blocks()
  invocation. Looks like it reserves enough blocks (but I propose you
  to check it, I don't know).

2) Even if that fails, the tree to that moment is already partially
  changed. And if you return -1 here, it is corrupted. To test that I
  added an error injection to bps_tree_process_insert_leaf() in this
  place. Initially it was false. I called box.cfg{}. Then I set it
  true. And created a space, an index. Tried to insert 100 numbers.
  It failed due to OOM. Then I turned the errinj off again, inserted
  the values again, and made a select. I got broken order of the
  select, and values returned multiple times.

Here is what I have done:

================================================================================
--- a/src/lib/salad/bps_tree.h
+++ b/src/lib/salad/bps_tree.h
@@ -38,6 +38,8 @@
 #include <stdio.h> /* printf */
 #include "small/matras.h"
 
+extern int bps_errinj;
+
 /* {{{ BPS-tree description */
 /**
  * BPS-tree implementation.
@@ -3716,6 +3718,8 @@ bps_tree_process_insert_leaf(struct bps_tree *tree,
                                leaf_path_elem, &new_path_elem, mc1, new_elem);
 
                bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
+               if (bps_errinj)
+                       return -1;
                struct bps_inner *new_root = bps_tree_create_inner(tree,
                                &new_root_id);
                if (!new_root)
================================================================================
Also you need to declare bps_errinj somewhere in a .c file.

Here is the test:

    box.cfg{}

    -- In lldb/gdb make bps_errinj variable 1.
    s = box.schema.create_space('test')
    _ = s:create_index('pk')
    for i = 1, 100 do s:replace{i} end

    -- This was an error. Now make bps_errinj 0 again.
    for i = 1, 100 do s:replace{i} end
    s:select{}

The select will return tuples in a wrong order.

>  		new_root->header.size = 2;
>  		new_root->child_ids[0] = tree->root_id;
>  		new_root->child_ids[1] = new_block_id;
> @@ -4030,6 +4042,8 @@ bps_tree_process_insert_inner(struct bps_tree *tree,
>  		bps_tree_block_id_t new_root_id = (bps_tree_block_id_t)(-1);
>  		struct bps_inner *new_root =
>  			bps_tree_create_inner(tree, &new_root_id);
> +		if (!new_root)
> +			return -1;

3. The same problem is here. You can't return from now on, the
tree is changed already.

>  		new_root->header.size = 2;
>  		new_root->child_ids[0] = tree->root_id;
>  		new_root->child_ids[1] = new_block_id;
>