[PATCH v2 0/2] fix bodiless requests handling.

[PATCH v2 0/2] fix bodiless requests handling.

[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

Changes in v2:
  - make sure that tarantoolctl cat and play
    stops on all records with lsn >= to, even
    the ones that have to be skipped.
  - move NOP recovery test to box/before_replace.test.lua
  - decided to ignore old xlogs with NOPS with body.
  - simplify check for NOP in xrow_header_decode()


Serge Petrenko (2):
  tarantoolctl: fix cat and play for empty body requests
  recovery: fix incorrect handling of empty-body requests.

 extra/dist/tarantoolctl.in         | 32 ++++++++++++++++++------------
 src/box/xrow.c                     |  3 ++-
 test/app-tap/tarantoolctl.test.lua | 14 ++++++++++---
 test/box/before_replace.result     | 25 ++++++++++++++++++++---
 test/box/before_replace.test.lua   | 14 ++++++++++---
 5 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.17.1 (Apple Git-112)

[PATCH v2 1/2] tarantoolctl: fix cat and play for empty body requests

[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, and, while we're at it,
make sure `play` and `cat` do not read excess rows with lsn>=to in case
these rows are skipped.

Closes #3675
---
 extra/dist/tarantoolctl.in         | 32 ++++++++++++++++++------------
 test/app-tap/tarantoolctl.test.lua | 14 ++++++++++---
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 3d7ff3ec5..588ca1729 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -373,6 +373,12 @@ write_lua_table = function(tuple)
 end
 
 local function cat_lua_cb(record)
+    -- Ignore both versions of IPROTO_NOP: the one without a
+    -- body (new), and the one with empty body (old).
+    if record.HEADER.type == 'NOP' or record.BODY == nil or
+       record.BODY.space_id == nil 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,17 +822,17 @@ 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
+            if lsn >= to then
+                -- stop, as we've finished reading tuple with lsn == to
+                -- and the next lsn's will be bigger
+                break
+            elseif (lsn < from) 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
                (replicas and not find_in_list(rid, replicas)) then
                 -- pass this tuple
-            elseif lsn >= to then
-                -- stop, as we've finished reading tuple with lsn == to
-                -- and the next lsn's will be bigger
-                break
             else
                 is_printed = true
                 format_cb(record)
@@ -857,17 +863,17 @@ 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
-               (not spaces and sid and sid < 512 and not show_system) or
-               (spaces and (sid == nil or not find_in_list(sid, spaces))) or
-               (replicas and not find_in_list(rid, replicas)) then
-                -- pass this tuple
-            elseif lsn >= to then
+            if lsn >= to then
                 -- stop, as we've finished reading tuple with lsn == to
                 -- and the next lsn's will be bigger
                 break
+            elseif (lsn < from) or sid == nil or
+               (not spaces and sid < 512 and not show_system) or
+               (spaces and not find_in_list(sid, spaces)) or
+               (replicas and not find_in_list(rid, replicas)) then
+                -- pass this tuple
             else
                 local args, so = {}, remote.space[sid]
                 if so == nil 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.17.1 (Apple Git-112)

[PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests.

[Serge Petrenko]

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)

Re: [PATCH v2 0/2] fix bodiless requests handling.

[Vladimir Davydov]

I refactored the test case added by the second patch slightly so as to
avoid restarting the server for a second time and pushed both patches to
1.10.

Re: [tarantool-patches] [PATCH v2 1/2] tarantoolctl: fix cat and play for empty body requests

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/09/24 18:11]:
> 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, and, while we're at it,
> make sure `play` and `cat` do not read excess rows with lsn>=to in case
> these rows are skipped.
> 
> Closes #3675
> ---
>  extra/dist/tarantoolctl.in         | 32 ++++++++++++++++++------------
>  test/app-tap/tarantoolctl.test.lua | 14 ++++++++++---
>  2 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index 3d7ff3ec5..588ca1729 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
> @@ -373,6 +373,12 @@ write_lua_table = function(tuple)
>  end
>  
>  local function cat_lua_cb(record)
> +    -- Ignore both versions of IPROTO_NOP: the one without a
> +    -- body (new), and the one with empty body (old).
> +    if record.HEADER.type == 'NOP' or record.BODY == nil or
> +       record.BODY.space_id == nil then
> +        return
> +    end

This comment doesn't match the code. You ignore all records with
an empty body, not just NOP records. This is obviously incorrect.

> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests.

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/09/24 18:11]:
> 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");

This patch LGTM.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov