* [PATCH 0/2] fix bodiless requests handling.
@ 2018-09-14 14:36 Serge Petrenko
2018-09-14 14:36 ` [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests Serge Petrenko
2018-09-14 14:36 ` [PATCH 2/2] recovery: fix incorrect handling of empty-body requests Serge Petrenko
0 siblings, 2 replies; 5+ messages in thread
From: Serge Petrenko @ 2018-09-14 14:36 UTC (permalink / raw)
To: vdavydov.dev; +Cc: tarantool-patches, Serge Petrenko
The first patch fixes an error when trying to parse a bodiless request
such as IPROTO_NOP in `tarantoolctl cat` and `tarantoolctl play`.
Now cat displays such requests correctly and play ignores them.
https://github.com/tarantool/tarantool/issues/3675
The second patch fixes parsing xlogs containing transactions with
empty body requests. Such requests weren't handled correctly which lead to
header of the next request become body of a no-op request. This messed up recovery
and `tarantoolctl cat`. Both fixed.
https://github.com/tarantool/tarantool/issues/3678
Branch:
https://github.com/tarantool/tarantool/tree/sp/gh-3675-tarantoolctl-cat-empty-body
Serge Petrenko (2):
tarantoolctl: fix cat and play for empty body requests
recovery: fix incorrect handling of empty-body requests.
extra/dist/tarantoolctl.in | 11 +++---
src/box/xrow.c | 6 +++-
test/app-tap/tarantoolctl.test.lua | 14 ++++++--
test/xlog/recover_nop.result | 72 ++++++++++++++++++++++++++++++++++++++
test/xlog/recover_nop.test.lua | 28 +++++++++++++++
5 files changed, 123 insertions(+), 8 deletions(-)
create mode 100644 test/xlog/recover_nop.result
create mode 100644 test/xlog/recover_nop.test.lua
--
2.15.2 (Apple Git-101.1)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests
2018-09-14 14:36 [PATCH 0/2] fix bodiless requests handling Serge Petrenko
@ 2018-09-14 14:36 ` Serge Petrenko
2018-09-14 15:37 ` Vladimir Davydov
2018-09-14 14:36 ` [PATCH 2/2] recovery: fix incorrect handling of empty-body requests Serge Petrenko
1 sibling, 1 reply; 5+ messages in thread
From: Serge Petrenko @ 2018-09-14 14:36 UTC (permalink / raw)
To: vdavydov.dev; +Cc: tarantool-patches, Serge Petrenko
If space.before_replace returns the old tuple, the operation turns into
no-op, but is still written to WAL as IPROTO_NOP for the sake of
replication. Such a request doesn't have a body, and tarantoolctl failed
to parse such requests in `tarantoolctl cat` and `tarantoolctl play`.
Fix this by checking whether a request has a body. Also skip such
requests in `play`, since they have no effect.
Closes #3675
---
| 11 +++++++----
test/app-tap/tarantoolctl.test.lua | 14 +++++++++++---
2 files changed, 18 insertions(+), 7 deletions(-)
--git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 3d7ff3ec5..a6bd15e95 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -373,6 +373,9 @@ write_lua_table = function(tuple)
end
local function cat_lua_cb(record)
+ if record.HEADER.type == 'NOP' then
+ return
+ end
io.stdout:write(('box.space[%d]'):format(record.BODY.space_id))
local op = record.HEADER.type:lower()
io.stdout:write((':%s('):format(op))
@@ -816,7 +819,7 @@ local function cat()
for id, file in ipairs(positional_arguments) do
log.error("Processing file '%s'", file)
for lsn, record in xlog.pairs(file) do
- local sid = record.BODY.space_id
+ local sid = record.BODY and record.BODY.space_id
local rid = record.HEADER.replica_id
if (lsn < from) or
(not spaces and sid and sid < 512 and not show_system) or
@@ -857,11 +860,11 @@ local function play()
for id, file in ipairs(positional_arguments) do
log.info(("Processing file '%s'"):format(file))
for lsn, record in xlog.pairs(file) do
- local sid = record.BODY.space_id
+ local sid = record.BODY and record.BODY.space_id
local rid = record.HEADER.replica_id
- if (lsn < from) or
+ if (lsn < from) or sid == nil or
(not spaces and sid and sid < 512 and not show_system) or
- (spaces and (sid == nil or not find_in_list(sid, spaces))) or
+ (spaces and not find_in_list(sid, spaces)) or
(replicas and not find_in_list(rid, replicas)) then
-- pass this tuple
elseif lsn >= to then
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 340232ace..458e6c030 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -345,6 +345,10 @@ do
space:update({[1] = 2}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
space:upsert({[1] = 3, [2] = 4, [3] = 5, [4] = 6}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
space:upsert({[1] = 3, [2] = 4, [3] = 5, [4] = 6}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
+ f = function(old, new) return old end
+ space:before_replace(f)
+ space:replace{1,5}
+ space:before_replace(nil, f)
os.exit(0)
]]
@@ -372,11 +376,11 @@ do
test:test("fill and test cat output", function(test_i)
test_i:plan(29)
check_ok(test_i, dir, 'start', 'filler', 0)
- check_ctlcat_xlog(test_i, dir, nil, "---\n", 6)
+ check_ctlcat_xlog(test_i, dir, nil, "---\n", 7)
check_ctlcat_xlog(test_i, dir, "--space=512", "---\n", 6)
check_ctlcat_xlog(test_i, dir, "--space=666", "---\n", 0)
- check_ctlcat_xlog(test_i, dir, "--show-system", "---\n", 9)
- check_ctlcat_xlog(test_i, dir, "--format=json", "\n", 6)
+ check_ctlcat_xlog(test_i, dir, "--show-system", "---\n", 10)
+ check_ctlcat_xlog(test_i, dir, "--format=json", "\n", 7)
check_ctlcat_xlog(test_i, dir, "--format=lua", "\n", 6)
check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json", "\n", 2)
check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system", "\n", 3)
@@ -411,6 +415,10 @@ do
space:update({[1] = 2}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
space:upsert({[1] = 3, [2] = 4, [3] = 5, [4] = 6}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
space:upsert({[1] = 3, [2] = 4, [3] = 5, [4] = 6}, {[1] = {[1] = '\x3d', [2] = 3, [3] = 4}})
+ f = function(old, new) return old end
+ space:before_replace(f)
+ space:replace{1,5}
+ space:before_replace(nil, f)
os.exit(0)
]]
create_script(dir, 'filler.lua', filler_code)
--
2.15.2 (Apple Git-101.1)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] recovery: fix incorrect handling of empty-body requests.
2018-09-14 14:36 [PATCH 0/2] fix bodiless requests handling Serge Petrenko
2018-09-14 14:36 ` [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests Serge Petrenko
@ 2018-09-14 14:36 ` Serge Petrenko
2018-09-14 16:27 ` Vladimir Davydov
1 sibling, 1 reply; 5+ messages in thread
From: Serge Petrenko @ 2018-09-14 14:36 UTC (permalink / raw)
To: vdavydov.dev; +Cc: tarantool-patches, Serge Petrenko
In some cases no-ops are written to xlog. They have no effect but are
needed to bump lsn. Such ops have no body, 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-ops 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 | 6 +++-
test/xlog/recover_nop.result | 72 ++++++++++++++++++++++++++++++++++++++++++
test/xlog/recover_nop.test.lua | 28 ++++++++++++++++
3 files changed, 105 insertions(+), 1 deletion(-)
create mode 100644 test/xlog/recover_nop.result
create mode 100644 test/xlog/recover_nop.test.lua
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 7a35d0db1..99dce5395 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -99,6 +99,8 @@ error:
if (mp_typeof(**pos) != MP_MAP)
goto error;
+ bool expect_body = true;
+
uint32_t size = mp_decode_map(pos);
for (uint32_t i = 0; i < size; i++) {
if (mp_typeof(**pos) != MP_UINT)
@@ -110,6 +112,8 @@ error:
switch (key) {
case IPROTO_REQUEST_TYPE:
header->type = mp_decode_uint(pos);
+ if (header->type == IPROTO_NOP)
+ expect_body = false;
break;
case IPROTO_SYNC:
header->sync = mp_decode_uint(pos);
@@ -135,7 +139,7 @@ error:
}
}
assert(*pos <= end);
- if (*pos < end) {
+ if (*pos < end && expect_body) {
const char *body = *pos;
if (mp_check(pos, end)) {
diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
diff --git a/test/xlog/recover_nop.result b/test/xlog/recover_nop.result
new file mode 100644
index 000000000..e6aad41a7
--- /dev/null
+++ b/test/xlog/recover_nop.result
@@ -0,0 +1,72 @@
+test_run = require('test_run').new()
+---
+...
+test_run:cmd('restart server default with cleanup=1')
+-- gh-3678 check for correct recovery with nops in transaction.
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:replace{1,1}
+---
+- [1, 1]
+...
+f = function(old, new) return old end
+---
+...
+box.begin() s:before_replace(f) s:replace{1,2} s:before_replace(nil, f) s:replace{1,3} box.commit()
+---
+...
+s:select{}
+---
+- - [1, 3]
+...
+test_run:cmd('restart server default')
+xlog = require('xlog')
+---
+...
+fio = require('fio')
+---
+...
+box.space.test:select{}
+---
+- - [1, 3]
+...
+xlog_path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', 0))
+---
+...
+fun, param, state = xlog.pairs(xlog_path)
+---
+...
+repeat state, row = fun(param, state) until row.HEADER.type == 'NOP'
+---
+...
+row.BODY == nil
+---
+- true
+...
+state, row = fun(param, state)
+---
+...
+row.HEADER.lsn
+---
+- 6
+...
+row.HEADER.replica_id
+---
+- 1
+...
+row.HEADER.type
+---
+- REPLACE
+...
+row.BODY
+---
+- space_id: 512
+ tuple: [1, 3]
+...
+box.space.test:drop()
+---
+...
diff --git a/test/xlog/recover_nop.test.lua b/test/xlog/recover_nop.test.lua
new file mode 100644
index 000000000..de8f8ef42
--- /dev/null
+++ b/test/xlog/recover_nop.test.lua
@@ -0,0 +1,28 @@
+test_run = require('test_run').new()
+
+test_run:cmd('restart server default with cleanup=1')
+-- gh-3678 check for correct recovery with nops in transaction.
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+s:replace{1,1}
+f = function(old, new) return old end
+box.begin() s:before_replace(f) s:replace{1,2} s:before_replace(nil, f) s:replace{1,3} box.commit()
+s:select{}
+
+test_run:cmd('restart server default')
+
+xlog = require('xlog')
+fio = require('fio')
+
+box.space.test:select{}
+xlog_path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', 0))
+fun, param, state = xlog.pairs(xlog_path)
+repeat state, row = fun(param, state) until row.HEADER.type == 'NOP'
+row.BODY == nil
+state, row = fun(param, state)
+row.HEADER.lsn
+row.HEADER.replica_id
+row.HEADER.type
+row.BODY
+
+box.space.test:drop()
--
2.15.2 (Apple Git-101.1)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests
2018-09-14 14:36 ` [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests Serge Petrenko
@ 2018-09-14 15:37 ` Vladimir Davydov
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Davydov @ 2018-09-14 15:37 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tarantool-patches
On Fri, Sep 14, 2018 at 05:36:40PM +0300, Serge Petrenko wrote:
> If space.before_replace returns the old tuple, the operation turns into
> no-op, but is still written to WAL as IPROTO_NOP for the sake of
> replication. Such a request doesn't have a body, and tarantoolctl failed
> to parse such requests in `tarantoolctl cat` and `tarantoolctl play`.
> Fix this by checking whether a request has a body. Also skip such
> requests in `play`, since they have no effect.
>
> Closes #3675
> ---
> extra/dist/tarantoolctl.in | 11 +++++++----
> test/app-tap/tarantoolctl.test.lua | 14 +++++++++++---
> 2 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index 3d7ff3ec5..a6bd15e95 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
> @@ -373,6 +373,9 @@ write_lua_table = function(tuple)
> end
>
> local function cat_lua_cb(record)
> + if record.HEADER.type == 'NOP' then
> + return
> + end
IMO better check 'record.BODY.space_id' instead of the type, because the
code below tries to use it.
> io.stdout:write(('box.space[%d]'):format(record.BODY.space_id))
> local op = record.HEADER.type:lower()
> io.stdout:write((':%s('):format(op))
> @@ -816,7 +819,7 @@ local function cat()
> for id, file in ipairs(positional_arguments) do
> log.error("Processing file '%s'", file)
> for lsn, record in xlog.pairs(file) do
> - local sid = record.BODY.space_id
> + local sid = record.BODY and record.BODY.space_id
> local rid = record.HEADER.replica_id
> if (lsn < from) or
> (not spaces and sid and sid < 512 and not show_system) or
> @@ -857,11 +860,11 @@ local function play()
> for id, file in ipairs(positional_arguments) do
> log.info(("Processing file '%s'"):format(file))
> for lsn, record in xlog.pairs(file) do
> - local sid = record.BODY.space_id
> + local sid = record.BODY and record.BODY.space_id
> local rid = record.HEADER.replica_id
> - if (lsn < from) or
> + if (lsn < from) or sid == nil or
> (not spaces and sid and sid < 512 and not show_system) or
'and sid' is useless after this change
Anyway, strictly speaking this is incorrect, because you won't stop on
a NOP record with lsn >= to, as you're supposed to.
> - (spaces and (sid == nil or not find_in_list(sid, spaces))) or
> + (spaces and not find_in_list(sid, spaces)) or
> (replicas and not find_in_list(rid, replicas)) then
> -- pass this tuple
> elseif lsn >= to then
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] recovery: fix incorrect handling of empty-body requests.
2018-09-14 14:36 ` [PATCH 2/2] recovery: fix incorrect handling of empty-body requests Serge Petrenko
@ 2018-09-14 16:27 ` Vladimir Davydov
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Davydov @ 2018-09-14 16:27 UTC (permalink / raw)
To: Serge Petrenko; +Cc: tarantool-patches
On Fri, Sep 14, 2018 at 05:36:41PM +0300, Serge Petrenko wrote:
> In some cases no-ops are written to xlog. They have no effect but are
> needed to bump lsn. Such ops have no body, 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-ops 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 | 6 +++-
> test/xlog/recover_nop.result | 72 ++++++++++++++++++++++++++++++++++++++++++
> test/xlog/recover_nop.test.lua | 28 ++++++++++++++++
> 3 files changed, 105 insertions(+), 1 deletion(-)
> create mode 100644 test/xlog/recover_nop.result
> create mode 100644 test/xlog/recover_nop.test.lua
>
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 7a35d0db1..99dce5395 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -99,6 +99,8 @@ error:
> if (mp_typeof(**pos) != MP_MAP)
> goto error;
>
> + bool expect_body = true;
> +
> uint32_t size = mp_decode_map(pos);
> for (uint32_t i = 0; i < size; i++) {
> if (mp_typeof(**pos) != MP_UINT)
> @@ -110,6 +112,8 @@ error:
> switch (key) {
> case IPROTO_REQUEST_TYPE:
> header->type = mp_decode_uint(pos);
> + if (header->type == IPROTO_NOP)
> + expect_body = false;
> break;
> case IPROTO_SYNC:
> header->sync = mp_decode_uint(pos);
> @@ -135,7 +139,7 @@ error:
> }
> }
> assert(*pos <= end);
> - if (*pos < end) {
> + if (*pos < end && expect_body) {
Why not simply
/* NOP request isn't supposed to have a body. */
if (*pos < end && header->type != IPROTO_NOP) {
?
Anyway, there's a problem here: NOP requests used to have a body, see
commit 89e5b7846c9d ("xrow: make NOP requests bodiless"), i.e. this
function has to decode NOP requests with and without a body.
I don't know what to do about it, because we can't discern a BODY from a
HEADER in an xlog without decoding it. Solutions I can see:
1. Assume nobody has ever used before_replace and hence there shouldn't
be NOP requests in old xlogs. Then this patch would be OK.
2. Take a peek inside the next map and try to determine whether it's a
body or a header.
3. Introduce a separate op-code for NOP without body.
> diff --git a/test/xlog/recover_nop.result b/test/xlog/recover_nop.result
> new file mode 100644
NOP recovery is already tested in box/before_replace.test.lua.
I think you'd better extend that rather than introducing a new test.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-09-14 16:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-14 14:36 [PATCH 0/2] fix bodiless requests handling Serge Petrenko
2018-09-14 14:36 ` [PATCH 1/2] tarantoolctl: fix cat and play for empty body requests Serge Petrenko
2018-09-14 15:37 ` Vladimir Davydov
2018-09-14 14:36 ` [PATCH 2/2] recovery: fix incorrect handling of empty-body requests Serge Petrenko
2018-09-14 16:27 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox