From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Serge Petrenko Subject: [PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests. Date: Mon, 24 Sep 2018 17:08:09 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: To: vdavydov.dev@gmail.com Cc: tarantool-patches@freelists.org, Serge Petrenko List-ID: In some cases no-ops are written to xlog. They have no effect but are needed to bump lsn. Some time ago (see commit 89e5b7846c9de968f8250ad03a128fc3b048271f) such ops were made bodiless, and empty body requests are not handled in xrow_header_decode(). This leads to recovery errors in special case: when we have a multi-statement transaction containing no-ops written to xlog, upon recovering from such xlog, all data after the no-op end till the start of new transaction will become no-op's body, so, effectively, it will be ignored. Here's example `tarantoolctl cat` output showing this (BODY contains next request data): ``` --- HEADER: lsn: 5 replica_id: 1 type: NOP timestamp: 1536656270.5092 BODY: type: 3 timestamp: 1536656270.5092 lsn: 6 replica_id: 1 --- HEADER: type: 0 ... ``` This patch handles no-ops correctly in xrow_header_decode(). Closes #3678 --- src/box/xrow.c | 3 ++- test/box/before_replace.result | 25 ++++++++++++++++++++++--- test/box/before_replace.test.lua | 14 +++++++++++--- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/box/xrow.c b/src/box/xrow.c index 7a35d0db1..4473acfe3 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -135,7 +135,8 @@ error: } } assert(*pos <= end); - if (*pos < end) { + /* Nop requests aren't supposed to have a body. */ + if (*pos < end && header->type != IPROTO_NOP) { const char *body = *pos; if (mp_check(pos, end)) { diag_set(ClientError, ER_INVALID_MSGPACK, "packet body"); diff --git a/test/box/before_replace.result b/test/box/before_replace.result index cd0a3be1b..0adab4dd6 100644 --- a/test/box/before_replace.result +++ b/test/box/before_replace.result @@ -640,14 +640,17 @@ fio = require('fio') xlog = require('xlog') --- ... -type(s:before_replace(function(old, new) return old end)) +f = function(old, new) return old end +--- +... +type(s:before_replace(f)) --- - function ... -s:insert{1, 1} +box.begin() s:insert{1, 1} s:replace{1,2} s:before_replace(nil, f) s:replace{1,3} box.commit() --- ... -path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn - 1)) +path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn - 3)) --- ... fun, param, state = xlog.pairs(path) @@ -664,6 +667,22 @@ row.BODY == nil --- - true ... +-- gh-3678 recovery after IPROTO_NOP in a multi-statement transaction: +test_run:cmd('restart server default') +s = box.space.test +--- +... +s:select() +--- +- - [1, 3] +... +s:truncate() +--- +... +type(s:before_replace(function(old, new) return old end)) +--- +- function +... -- gh-3128 before_replace with run_triggers s2 = box.schema.space.create("test2") --- diff --git a/test/box/before_replace.test.lua b/test/box/before_replace.test.lua index dd4643844..919d2ccbb 100644 --- a/test/box/before_replace.test.lua +++ b/test/box/before_replace.test.lua @@ -214,15 +214,23 @@ s:select() fio = require('fio') xlog = require('xlog') -type(s:before_replace(function(old, new) return old end)) -s:insert{1, 1} +f = function(old, new) return old end +type(s:before_replace(f)) +box.begin() s:insert{1, 1} s:replace{1,2} s:before_replace(nil, f) s:replace{1,3} box.commit() -path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn - 1)) +path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn - 3)) fun, param, state = xlog.pairs(path) state, row = fun(param, state) row.HEADER.type row.BODY == nil +-- gh-3678 recovery after IPROTO_NOP in a multi-statement transaction: +test_run:cmd('restart server default') +s = box.space.test +s:select() +s:truncate() +type(s:before_replace(function(old, new) return old end)) + -- gh-3128 before_replace with run_triggers s2 = box.schema.space.create("test2") _ = s2:create_index("prim") -- 2.17.1 (Apple Git-112)