<HTML><BODY>Hi<br>Old code covers only case with broken records.<br>During discussion with Konstantin and Roman we decided that situation with broken header in last xlog is almost impossible. And also there was a concern that if header is broken user can repair it. I think zero-size file must be deleted, as we can't get any useful data from them. And old code delete last xlog with broken records. That's why to cases for delete:<br>zero-size file or one with broken records. So yes, code in recovery finalize covers these two situations.<br>About to rename non-empty xlog .. well I think it discussible.<br>Konstantin, Roman, what do you think?<br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Понедельник, 29 января 2018, 13:42 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base target="_self" href="https://e.mail.ru/">
<div id="style_15172225400000001002_BODY">On Mon, Jan 29, 2018 at 12:52:39PM +0300, Konstantin Belyavskiy wrote:<br>
<br>
> diff --git a/src/box/recovery.cc b/src/box/recovery.cc<br>
> index 281ac1838..cef7f11bd 100644<br>
> --- a/src/box/recovery.cc<br>
> +++ b/src/box/recovery.cc<br>
<br>
> @@ -330,20 +332,24 @@ recovery_finalize(struct recovery *r, struct xstream *stream)<br>
> recovery_close_log(r);<br>
> <br>
> /*<br>
> - * Check that the last xlog file has rows.<br>
> + * Delete the last xlog file if either:<br>
> + * - file is corrupted<br>
> + * - file has zero size<br>
> */<br>
> - if (vclockset_last(&r->wal_dir.index) != NULL &&<br>
> - vclock_sum(&r->vclock) ==<br>
> - vclock_sum(vclockset_last(&r->wal_dir.index))) {<br>
> - /*<br>
> - * Delete the last empty xlog file.<br>
> - */<br>
> + if (vclockset_last(&r->wal_dir.index) != NULL) {<br>
> char *name = xdir_format_filename(&r->wal_dir,<br>
> vclock_sum(&r->vclock),<br>
> NONE);<br>
> - if (unlink(name) != 0) {<br>
> - tnt_raise(SystemError, "%s: failed to unlink file",<br>
> - name);<br>
> + struct stat st;<br>
> + if ((stat(name, &st) == 0 && st.st_size == 0) ||<br>
> + vclock_sum(&r->vclock) ==<br>
> + vclock_sum(vclockset_last(&r->wal_dir.index))) {<br>
<br>
I don't understand why you check the file size if you intend to delete<br>
the last xlog file in case we failed to recover any records from it.<br>
Doesn't the latter imply the former?<br>
<br>
Anyway, deletion of a non-empty file doesn't sound right to me.<br>
<br>
> + say_info("delete corrupted xlog %s", name);<br>
> + if (unlink(name) != 0) {<br>
> + tnt_raise(SystemError,<br>
> + "%s: failed to unlink file",<br>
> + name);<br>
> + }<br>
> }<br>
> }<br>
> }<br>
<br>
> diff --git a/test/xlog/skip_empty_and_delete_last_corrupted_xlog_on_recovery.test.lua b/test/xlog/skip_empty_and_delete_last_corrupted_xlog_on_recovery.test.lua<br>
> new file mode 100644<br>
> index 000000000..8e8ebe126<br>
> --- /dev/null<br>
> +++ b/test/xlog/skip_empty_and_delete_last_corrupted_xlog_on_recovery.test.lua<br>
<br>
Please call the test, xlog/force_recovery.test.lua. It's less verbose<br>
but still clear, and we won't have to rename it if we add yet another<br>
test case.<br>
<br>
> @@ -0,0 +1,42 @@<br>
> +#!/usr/bin/env tarantool<br>
> +<br>
> +env = require('test_run')<br>
> +test_run = env.new()<br>
> +<br>
> +box.cfg{}<br>
> +<br>
> +test_run:cmd('create server test with script = "xlog/force_recovery.lua"')<br>
> +<br>
> +test_run:cmd("start server test")<br>
> +test_run:cmd("switch test")<br>
> +box.space._schema:replace({'test'})<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server test")<br>
> +<br>
> +test_run:cmd("start server test")<br>
> +test_run:cmd("switch test")<br>
> +box.space._schema:replace({'lost'})<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server test")<br>
> +<br>
> +test_run:cmd("start server test")<br>
> +test_run:cmd("switch test")<br>
> +box.space._schema:replace({'tost'})<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server test")<br>
> +<br>
> +os.execute("rm force_recovery/00000000000000000001.xlog")<br>
> +os.execute("touch force_recovery/00000000000000000001.xlog")<br>
<br>
We have the fio module for working with files.<br>
<br>
> +<br>
> +test_run:cmd("start server test")<br>
> +test_run:cmd("switch test")<br>
> +box.space._schema:replace({'last'})<br>
> +test_run:cmd("switch default")<br>
> +test_run:cmd("stop server test")<br>
> +<br>
> +os.execute("rm force_recovery/00000000000000000003.xlog")<br>
> +os.execute("touch force_recovery/00000000000000000003.xlog")<br>
> +<br>
> +test_run:cmd("start server test")<br>
> +test_run:cmd("switch test")<br>
> +box.space._schema:replace({'list'})<br>
<br>
AFAICS you only test recovery in case the last xlog file is empty.<br>
If the last xlog file has a corrupted header, it doesn't seem to work<br>
as expected. Try running this script:<br>
<br>
box.cfg{}<br>
s = box.schema.space.create('test')<br>
_ = s:create_index('pk')<br>
box.snapshot()<br>
s:insert{1}<br>
-- Corrupt the last xlog file.<br>
fio = require 'fio'<br>
path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn - 1))<br>
f = fio.open(path, {'O_WRONLY'})<br>
f:write('DEAD')<br>
f:close()<br>
os.exit(0)<br>
<br>
Tarantool does recover if force_recovery is set, but any attempt to<br>
write to the database fails with:<br>
<br>
tarantool> box.space.test:insert{2}<br>
2018-01-29 13:36:20.520 [8308] wal/101/main xlog.c:701 !> SystemError file './00000000000000000003.xlog' already exists: File exists<br>
2018-01-29 13:36:20.520 [8308] main/101/interactive txn.c:271 E> ER_WAL_IO: Failed to write to disk<br>
---<br>
- error: Failed to write to disk<br>
...<br>
<br>
I think we should fix this, too.<br>
</div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div>
</div>
</blockquote>
<br>
<br>С уважением,<br>Konstantin Belyavskiy<br>k.belyavskiy@tarantool.org<br></BODY></HTML>