Re[2]: [patches] Re: [PATCH] Fix force_recovery on empty xlog

[Konstantin Belyavskiy]

Roman accepted idea with rename corrupted (not empty) xlog instead delete it.
New version.

Branch: gh-3026-fix-force-recovery-on-empty-xlog


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


С уважением,
Konstantin Belyavskiy
k.belyavskiy at tarantool.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180129/2544a2a0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-force_recovery-on-empty-xlog.patch
Type: application/x-patch
Size: 6858 bytes
Desc: not available
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180129/2544a2a0/attachment.bin>