Re: [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files

[Nikita Pettik <korablev@tarantool.org>]

On 13 Jan 14:27, Maksim Kulis wrote:
> While deletion .run files and recover with force_recovery=true we have to delete some logs which are responsible for deleted files so that we can recover later with force_recovery=false.

Please, limit commit message with 80 characters per line.

Is there any issue corresponding to the problem you are solving?
 
> ---
>  src/box/vy_lsm.c                   |  19 +-
>  test/vinyl/delete_run.result       | 302 +++++++++++++++++++++++++++++
>  test/vinyl/delete_run.test.lua     |  93 +++++++++
>  test/vinyl/force_recovery_true.lua |  11 ++
>  4 files changed, 421 insertions(+), 4 deletions(-)
>  create mode 100644 test/vinyl/delete_run.result
>  create mode 100644 test/vinyl/delete_run.test.lua
>  create mode 100644 test/vinyl/force_recovery_true.lua
> 
> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> index aa4bce9eb..a8abaf031 100644
> --- a/src/box/vy_lsm.c
> +++ b/src/box/vy_lsm.c
> @@ -374,6 +374,9 @@ vy_lsm_recover_run(struct vy_lsm *lsm, struct vy_run_recovery_info *run_info,
>  				  lsm->space_id, lsm->index_id,
>  				  lsm->cmp_def, lsm->key_def,
>  				  lsm->disk_format, &lsm->opts) != 0)) {
> +		vy_log_tx_begin();
> +		vy_log_drop_run(run_info->id, 0);
> +		vy_log_tx_try_commit();
>  		vy_run_unref(run);

You can re-use vy_run_discard().

>  		return NULL;
>  	}
> @@ -426,8 +429,14 @@ vy_lsm_recover_slice(struct vy_lsm *lsm, struct vy_range *range,
>  
>  	run = vy_lsm_recover_run(lsm, slice_info->run,
>  				 run_env, force_recovery);
> -	if (run == NULL)
> +	if (run == NULL) {
> +		if (force_recovery) {
> +			vy_log_tx_begin();
> +			vy_log_delete_slice(slice_info->id);
> +			vy_log_tx_try_commit();
> +		}
>  		goto out;
> +	}
>  
>  	slice = vy_slice_new(slice_info->id, run, begin, end, lsm->cmp_def);
>  	if (slice == NULL)
> @@ -486,9 +495,11 @@ vy_lsm_recover_range(struct vy_lsm *lsm,
>  	rlist_foreach_entry_reverse(slice_info, &range_info->slices, in_range) {
>  		if (vy_lsm_recover_slice(lsm, range, slice_info,
>  					 run_env, force_recovery) == NULL) {
> -			vy_range_delete(range);
> -			range = NULL;
> -			goto out;
> +			if (force_recovery == false) {

Nit: if (!force_recovery)

Why do we delete slice if force_recovery is enabled,
meanwhile delete range/run when force_recovery is disabled?

> +				vy_range_delete(range);
> +				range = NULL;
> +				goto out;
> +			}
>  		}
>  	}
>  	vy_lsm_add_range(lsm, range);
> diff --git a/test/vinyl/delete_run.test.lua b/test/vinyl/delete_run.test.lua
> new file mode 100644
> index 000000000..1e8ed39ed
> --- /dev/null
> +++ b/test/vinyl/delete_run.test.lua
> @@ -0,0 +1,93 @@
> +test_run = require('test_run').new()
> +fiber = require('fiber')
> +
> +-- Test 1

Could you comment these test cases explaining what's going on here?

> +test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
> +test_run:cmd("start server test with args='1'")
> +test_run:cmd("switch test")
> +
> +test = box.schema.space.create('test', {engine='vinyl'})
> +_ = test:create_index('pk')
> +for i = 1, 10 do test:insert{i, i + 100} box.snapshot() end
> +test:select()
> +
> +fio = require ('fio')
> +for _, f in pairs(fio.glob(box.cfg.vinyl_dir .. '/' .. test.id .. '/0/*26*')) do fio.unlink(f) end
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="1"')
> +test_run:cmd('switch test')
> +box.cfg.force_recovery
> +box.space.test:select()
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="0"')
> +test_run:cmd('switch test')
> +box.cfg.force_recovery
> +box.space.test:select()
> +
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +test_run:cmd('cleanup server test')
> +test_run:cmd('delete server test')
> +
> +-- Test 2
> +test_run = require('test_run').new()
> +test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
> +test_run:cmd("start server test with args='1'")
> +test_run:cmd("switch test")
> +
> +test = box.schema.space.create('test', {engine='vinyl'})
> +_ = test:create_index('pk')
> +_ = test:insert{1, "123"}
> +test:select()
> +box.snapshot()
> +
> +fio = require ('fio')
> +for _, f in pairs(fio.glob(box.cfg.vinyl_dir .. '/' .. test.id .. '/0/*.index')) do fio.unlink(f) end
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="1"')
> +test_run:cmd('switch test')
> +box.space.test:select()
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="0"')
> +test_run:cmd('switch test')
> +box.space.test:select()
> +
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +test_run:cmd('cleanup server test')
> +test_run:cmd('delete server test')
> +
> +-- Test3
> +
> +test_run = require('test_run').new()
> +test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
> +test_run:cmd("start server test")
> +test_run:cmd("switch test")
> +
> +test = box.schema.space.create('test', {engine='vinyl'})
> +_ = test:create_index('pk')
> +_ = test:insert{1, "123"}
> +test:select(1)
> +box.snapshot()
> +
> +fio = require ('fio')
> +for _, f in pairs(fio.glob(box.cfg.vinyl_dir .. '/' .. test.id .. '/0/*.run')) do fio.unlink(f) end
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="1"')
> +test_run:cmd('switch test')
> +box.space.test:select()
> +
> +test_run:cmd('switch default')
> +test_run:cmd('restart server test with args="0"')
> +test_run:cmd('switch test')
> +box.cfg.force_recovery
> +box.space.test:select()
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +test_run:cmd('cleanup server test')
> +test_run:cmd('delete server test')
> diff --git a/test/vinyl/force_recovery_true.lua b/test/vinyl/force_recovery_true.lua
> new file mode 100644
> index 000000000..53414f22c
> --- /dev/null
> +++ b/test/vinyl/force_recovery_true.lua
> @@ -0,0 +1,11 @@
> +#!/usr/bin/env tarantool
> +
> +local fr = tonumber(arg[1])
> +box.cfg ({
> +    listen = os.getenv("LISTEN"),
> +    replication = os.getenv("MASTER"),
> +    vinyl_memory = 128 * 1024 * 1024,
> +    force_recovery = (fr == 1),
> +})
> +
> +require('console').listen(os.getenv('ADMIN'))
> -- 
> 2.17.1
>