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 2/2] recovery: fix incorrect handling of empty-body requests.
Date: Fri, 14 Sep 2018 17:36:41 +0300	[thread overview]
Message-ID: <724c197f81170aac18d37d976c835177925959f6.1536934854.git.sergepetrenko@tarantool.org> (raw)
In-Reply-To: <cover.1536934854.git.sergepetrenko@tarantool.org>
In-Reply-To: <cover.1536934854.git.sergepetrenko@tarantool.org>

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)

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

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 ` Serge Petrenko [this message]
2018-09-14 16:27   ` [PATCH 2/2] recovery: fix incorrect handling of empty-body requests 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=724c197f81170aac18d37d976c835177925959f6.1536934854.git.sergepetrenko@tarantool.org \
    --to=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH 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