Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files
@ 2020-01-13 11:27 Maksim Kulis
  2020-01-15 20:38 ` Nikita Pettik
  0 siblings, 1 reply; 3+ messages in thread
From: Maksim Kulis @ 2020-01-13 11:27 UTC (permalink / raw)
  To: tarantool-patches

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.

---
 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);
 		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) {
+				vy_range_delete(range);
+				range = NULL;
+				goto out;
+			}
 		}
 	}
 	vy_lsm_add_range(lsm, range);
diff --git a/test/vinyl/delete_run.result b/test/vinyl/delete_run.result
new file mode 100644
index 000000000..455545055
--- /dev/null
+++ b/test/vinyl/delete_run.result
@@ -0,0 +1,302 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+
+-- Test 1
+test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server test with args='1'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("switch test")
+ | ---
+ | - true
+ | ...
+
+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()
+ | ---
+ | - - [1, 101]
+ |   - [2, 102]
+ |   - [3, 103]
+ |   - [4, 104]
+ |   - [5, 105]
+ |   - [6, 106]
+ |   - [7, 107]
+ |   - [8, 108]
+ |   - [9, 109]
+ |   - [10, 110]
+ | ...
+
+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')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="1"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.cfg.force_recovery
+ | ---
+ | - true
+ | ...
+box.space.test:select()
+ | ---
+ | - - [9, 109]
+ |   - [10, 110]
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="0"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.cfg.force_recovery
+ | ---
+ | - false
+ | ...
+box.space.test:select()
+ | ---
+ | - - [9, 109]
+ |   - [10, 110]
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('cleanup server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server test')
+ | ---
+ | - true
+ | ...
+
+-- Test 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server test with args='1'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("switch test")
+ | ---
+ | - true
+ | ...
+
+test = box.schema.space.create('test', {engine='vinyl'})
+ | ---
+ | ...
+_ = test:create_index('pk')
+ | ---
+ | ...
+_ = test:insert{1, "123"}
+ | ---
+ | ...
+test:select()
+ | ---
+ | - - [1, '123']
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+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')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="1"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.space.test:select()
+ | ---
+ | - - [1, '123']
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="0"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.space.test:select()
+ | ---
+ | - - [1, '123']
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('cleanup server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server test')
+ | ---
+ | - true
+ | ...
+
+-- Test3
+
+test_run = require('test_run').new()
+ | ---
+ | ...
+test_run:cmd("create server test with script='vinyl/force_recovery_true.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server test")
+ | ---
+ | - true
+ | ...
+test_run:cmd("switch test")
+ | ---
+ | - true
+ | ...
+
+test = box.schema.space.create('test', {engine='vinyl'})
+ | ---
+ | ...
+_ = test:create_index('pk')
+ | ---
+ | ...
+_ = test:insert{1, "123"}
+ | ---
+ | ...
+test:select(1)
+ | ---
+ | - - [1, '123']
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+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')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="1"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.space.test:select()
+ | ---
+ | - []
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('restart server test with args="0"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+box.cfg.force_recovery
+ | ---
+ | - false
+ | ...
+box.space.test:select()
+ | ---
+ | - []
+ | ...
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('cleanup server test')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server test')
+ | ---
+ | - true
+ | ...
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
+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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files
  2020-01-13 11:27 [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files Maksim Kulis
@ 2020-01-15 20:38 ` Nikita Pettik
  2020-02-04  0:22   ` Maxim Kulis
  0 siblings, 1 reply; 3+ messages in thread
From: Nikita Pettik @ 2020-01-15 20:38 UTC (permalink / raw)
  To: Maksim Kulis; +Cc: tarantool-patches

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
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files
  2020-01-15 20:38 ` Nikita Pettik
@ 2020-02-04  0:22   ` Maxim Kulis
  0 siblings, 0 replies; 3+ messages in thread
From: Maxim Kulis @ 2020-02-04  0:22 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

[-- Attachment #1: Type: text/plain, Size: 13199 bytes --]


Hi!

> Is there any issue corresponding to the problem you are solving?

No, there is no issue corresponding to that problem.

> > ---
> > 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().

Yes, I agree.


> > 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?

Slice is a reference to a run containing a key range and it’s stored only in the metadata log.
So, if run file is deleted and we restart server with force_recovery=true we have to delete
the corresponding slice to ignore the file loss. 
Range is an LSM-subtree, so if we want to get the rest of the subtree  at any cost (force_recovery=true)
we don't have to delete the range.

> 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
bash: !/usr/bin/env: event not found
> > +
> > +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
> >



In these tests I make some snapshots and delete .run and .index files. 
Then I recover server with force_reovery = true,
and in the end I restart serverwith force_recovery = false to make sure that
the server will start up

At the first test I delete only one .run file one .index file.
The second test deletes all .index files and the third tests deletes all .run files.

branch:  https://github.com/maksimkulis/tarantool/tree/maksimkulis/recover-after-deletion-run-files  

>Среда, 15 января 2020, 23:38 +03:00 от 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
>> 


-- 
Maxim Kulis

[-- Attachment #2: Type: text/html, Size: 17417 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-02-04  0:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-13 11:27 [Tarantool-patches] [PATCH] vinyl: fix recovery after deletion .run files Maksim Kulis
2020-01-15 20:38 ` Nikita Pettik
2020-02-04  0:22   ` Maxim Kulis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox