Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko <sergepetrenko@tarantool.org>
To: vdavydov.dev@gmail.com
Cc: tarantool-patches@freelists.org,
	Serge Petrenko <sergepetrenko@tarantool.org>
Subject: [PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests.
Date: Mon, 24 Sep 2018 17:08:09 +0300	[thread overview]
Message-ID: <a4e80c4c9d1642e017559b1949b65360057085a0.1537797732.git.sergepetrenko@tarantool.org> (raw)
In-Reply-To: <cover.1537797732.git.sergepetrenko@tarantool.org>
In-Reply-To: <cover.1537797732.git.sergepetrenko@tarantool.org>

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)

  parent reply	other threads:[~2018-09-24 14:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-24 14:08 [PATCH v2 0/2] fix bodiless requests handling Serge Petrenko
2018-09-24 14:08 ` [PATCH v2 1/2] tarantoolctl: fix cat and play for empty body requests Serge Petrenko
2018-09-25 22:31   ` [tarantool-patches] " Konstantin Osipov
2018-09-24 14:08 ` Serge Petrenko [this message]
2018-09-25 22:33   ` [tarantool-patches] [PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests Konstantin Osipov
2018-09-25 14:37 ` [PATCH v2 0/2] fix bodiless requests handling Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a4e80c4c9d1642e017559b1949b65360057085a0.1537797732.git.sergepetrenko@tarantool.org \
    --to=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH v2 2/2] recovery: fix incorrect handling of empty-body requests.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox