* [PATCH 1/6] txn: remove unused C++ wrappers
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-27 17:08 ` Konstantin Osipov
2018-06-13 16:10 ` [PATCH 2/6] xrow: fix ret code on decode failure Vladimir Davydov
` (4 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
---
src/box/txn.h | 25 -------------------------
1 file changed, 25 deletions(-)
diff --git a/src/box/txn.h b/src/box/txn.h
index 4db74dfc..f3d690be 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -370,31 +370,6 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *savepoint);
#include "diag.h"
-static inline struct txn *
-txn_begin_stmt_xc(struct space *space)
-{
- struct txn *txn = txn_begin_stmt(space);
- if (txn == NULL)
- diag_raise();
- return txn;
-}
-
-static inline struct txn *
-txn_begin_ro_stmt_xc(struct space *space)
-{
- struct txn *txn;
- if (txn_begin_ro_stmt(space, &txn) != 0)
- diag_raise();
- return txn;
-}
-
-static inline void
-txn_commit_stmt_xc(struct txn *txn, struct request *request)
-{
- if (txn_commit_stmt(txn, request) != 0)
- diag_raise();
-}
-
static inline void
txn_check_singlestatement_xc(struct txn *txn, const char *where)
{
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 2/6] xrow: fix ret code on decode failure
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
2018-06-13 16:10 ` [PATCH 1/6] txn: remove unused C++ wrappers Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-27 17:29 ` Konstantin Osipov
2018-06-13 16:10 ` [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code Vladimir Davydov
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
Throughout the code, we return -1 on error, but decode methods return 1
for some reason, although according to comments they are supposed to
return -1. This doesn't result in any errors, because we use != 0 to
check for errors. Nevertheless, let's fix it to avoid confusion.
---
src/box/xrow.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 746913a4..48fbff27 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -408,7 +408,7 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
if (row->bodycnt == 0) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"missing request body");
- return 1;
+ return -1;
}
assert(row->bodycnt == 1);
@@ -601,7 +601,7 @@ xrow_decode_call(const struct xrow_header *row, struct call_request *request)
if (row->bodycnt == 0) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"missing request body");
- return 1;
+ return -1;
}
assert(row->bodycnt == 1);
@@ -612,7 +612,7 @@ xrow_decode_call(const struct xrow_header *row, struct call_request *request)
if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
error:
diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
- return 1;
+ return -1;
}
memset(request, 0, sizeof(*request));
@@ -651,20 +651,20 @@ error:
}
if (data != end) {
diag_set(ClientError, ER_INVALID_MSGPACK, "packet end");
- return 1;
+ return -1;
}
if (row->type == IPROTO_EVAL) {
if (request->expr == NULL) {
diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
iproto_key_name(IPROTO_EXPR));
- return 1;
+ return -1;
}
} else if (request->name == NULL) {
assert(row->type == IPROTO_CALL_16 ||
row->type == IPROTO_CALL);
diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
iproto_key_name(IPROTO_FUNCTION_NAME));
- return 1;
+ return -1;
}
if (request->args == NULL) {
static const char empty_args[] = { (char)0x90 };
@@ -680,7 +680,7 @@ xrow_decode_auth(const struct xrow_header *row, struct auth_request *request)
if (row->bodycnt == 0) {
diag_set(ClientError, ER_INVALID_MSGPACK,
"missing request body");
- return 1;
+ return -1;
}
assert(row->bodycnt == 1);
@@ -691,7 +691,7 @@ xrow_decode_auth(const struct xrow_header *row, struct auth_request *request)
if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
error:
diag_set(ClientError, ER_INVALID_MSGPACK, "packet body");
- return 1;
+ return -1;
}
memset(request, 0, sizeof(*request));
@@ -723,17 +723,17 @@ error:
}
if (data != end) {
diag_set(ClientError, ER_INVALID_MSGPACK, "packet end");
- return 1;
+ return -1;
}
if (request->user_name == NULL) {
diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
iproto_key_name(IPROTO_USER_NAME));
- return 1;
+ return -1;
}
if (request->scramble == NULL) {
diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
iproto_key_name(IPROTO_TUPLE));
- return 1;
+ return -1;
}
return 0;
}
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/6] xrow: fix ret code on decode failure
2018-06-13 16:10 ` [PATCH 2/6] xrow: fix ret code on decode failure Vladimir Davydov
@ 2018-06-27 17:29 ` Konstantin Osipov
0 siblings, 0 replies; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-27 17:29 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> Throughout the code, we return -1 on error, but decode methods return 1
> for some reason, although according to comments they are supposed to
> return -1. This doesn't result in any errors, because we use != 0 to
> check for errors. Nevertheless, let's fix it to avoid confusion.
> ---
> src/box/xrow.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Will push shortly.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
2018-06-13 16:10 ` [PATCH 1/6] txn: remove unused C++ wrappers Vladimir Davydov
2018-06-13 16:10 ` [PATCH 2/6] xrow: fix ret code on decode failure Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-27 17:48 ` Konstantin Osipov
2018-06-13 16:10 ` [PATCH 4/6] txn: do not require space id for nop requests Vladimir Davydov
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
IPROTO_SERVER_IS_RO currently has code 0x07 and is defined in the header
key section, which is wrong, because this key is only used in request
body. Let's move it to the body section, where it belongs, and set its
code to 0x29. This shouldn't break anything even if 0x07 is reused in
future, because the two codes belong to different sections and hence are
never parsed in the same function. Worst that can happen is we fail to
bootstrap a node in the cluster if it is running a newer tarantool
version.
While we are at it, let's also add the key name and change its type from
MP_UINT to MP_BOOL.
Fixes commit a8ecd1e122ae ("replication: fix bug with read-only replica
as a bootstrap leader").
---
src/box/iproto_constants.c | 5 +++--
src/box/iproto_constants.h | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/box/iproto_constants.c b/src/box/iproto_constants.c
index 6dba85f5..3adb7cd4 100644
--- a/src/box/iproto_constants.c
+++ b/src/box/iproto_constants.c
@@ -40,10 +40,10 @@ const unsigned char iproto_key_type[IPROTO_KEY_MAX] =
/* 0x04 */ MP_DOUBLE, /* IPROTO_TIMESTAMP */
/* 0x05 */ MP_UINT, /* IPROTO_SCHEMA_VERSION */
/* 0x06 */ MP_UINT, /* IPROTO_SERVER_VERSION */
- /* 0x07 */ MP_UINT, /* IPROTO_SERVER_IS_RO */
/* }}} */
/* {{{ unused */
+ /* 0x07 */ MP_UINT,
/* 0x08 */ MP_UINT,
/* 0x09 */ MP_UINT,
/* 0x0a */ MP_UINT,
@@ -86,6 +86,7 @@ const unsigned char iproto_key_type[IPROTO_KEY_MAX] =
/* 0x26 */ MP_MAP, /* IPROTO_VCLOCK */
/* 0x27 */ MP_STR, /* IPROTO_EXPR */
/* 0x28 */ MP_ARRAY, /* IPROTO_OPS */
+ /* 0x29 */ MP_BOOL, /* IPROTO_SERVER_IS_RO */
/* }}} */
};
@@ -166,7 +167,7 @@ const char *iproto_key_strs[IPROTO_KEY_MAX] = {
"vector clock", /* 0x26 */
"expression", /* 0x27 */
"operations", /* 0x28 */
- NULL, /* 0x29 */
+ "server is ro", /* 0x29 */
NULL, /* 0x2a */
NULL, /* 0x2b */
NULL, /* 0x2c */
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index dd50bf87..d1320de7 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -58,7 +58,6 @@ enum iproto_key {
IPROTO_TIMESTAMP = 0x04,
IPROTO_SCHEMA_VERSION = 0x05,
IPROTO_SERVER_VERSION = 0x06,
- IPROTO_SERVER_IS_RO = 0x07,
/* Leave a gap for other keys in the header. */
IPROTO_SPACE_ID = 0x10,
IPROTO_INDEX_ID = 0x11,
@@ -77,6 +76,7 @@ enum iproto_key {
IPROTO_VCLOCK = 0x26,
IPROTO_EXPR = 0x27, /* EVAL */
IPROTO_OPS = 0x28, /* UPSERT but not UPDATE ops, because of legacy */
+ IPROTO_SERVER_IS_RO = 0x29,
/* Leave a gap between request keys and response keys */
IPROTO_DATA = 0x30,
IPROTO_ERROR = 0x31,
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code
2018-06-13 16:10 ` [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code Vladimir Davydov
@ 2018-06-27 17:48 ` Konstantin Osipov
0 siblings, 0 replies; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-27 17:48 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> IPROTO_SERVER_IS_RO currently has code 0x07 and is defined in the header
> key section, which is wrong, because this key is only used in request
> body. Let's move it to the body section, where it belongs, and set its
> code to 0x29. This shouldn't break anything even if 0x07 is reused in
> future, because the two codes belong to different sections and hence are
> never parsed in the same function. Worst that can happen is we fail to
> bootstrap a node in the cluster if it is running a newer tarantool
> version.
Ugh, this is going to break things and we may have to revert, I
pushed to see how it goes.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/6] txn: do not require space id for nop requests
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
` (2 preceding siblings ...)
2018-06-13 16:10 ` [PATCH 3/6] iproto: fix IPROTO_SERVER_IS_RO key code Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-27 17:45 ` Konstantin Osipov
2018-06-13 16:10 ` [PATCH 5/6] xrow: make NOP requests bodiless Vladimir Davydov
2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov
5 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
Currently, IPROTO_NOP can only be generated by a before_replace trigger,
when it returns the old tuple thus turning the original operation into a
NOP. In such a case we know the space id and we write it to the request
body. This allows us to dispatch NOP requests via DML route.
As a part of replica local spaces feature, we will substitute requests
operating on local spaces with NOP in relay in order to promote vclock
on replicas without actual data modification. Since space_id is stored
in request body, sending it to replicas would mean decoding the request
body in relay, which is an overkill. To avoid that, let's separate NOP
and DML paths and remove space_id from NOP requests.
Needed for #3443
---
src/box/box.cc | 5 ++
src/box/iproto_constants.c | 2 +-
src/box/request.c | 2 +-
src/box/txn.c | 99 ++++++++++++++++++++++++++--------------
src/box/txn.h | 12 +++++
test/box/before_replace.result | 4 +-
test/box/before_replace.test.lua | 2 +-
7 files changed, 86 insertions(+), 40 deletions(-)
diff --git a/src/box/box.cc b/src/box/box.cc
index c1d15644..8248cc77 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -277,6 +277,11 @@ apply_row(struct xstream *stream, struct xrow_header *row)
(void) stream;
struct request request;
xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
+ if (request.type == IPROTO_NOP) {
+ if (txn_commit_nop(&request) != 0)
+ diag_raise();
+ return;
+ }
struct space *space = space_cache_find_xc(request.space_id);
if (process_rw(&request, space, NULL) != 0) {
say_error("error applying row: %s", request_str(&request));
diff --git a/src/box/iproto_constants.c b/src/box/iproto_constants.c
index 3adb7cd4..5c1d3a31 100644
--- a/src/box/iproto_constants.c
+++ b/src/box/iproto_constants.c
@@ -121,7 +121,7 @@ const uint64_t iproto_body_key_map[IPROTO_TYPE_STAT_MAX] = {
bit(SPACE_ID) | bit(OPS) | bit(TUPLE), /* UPSERT */
0, /* CALL */
0, /* reserved */
- bit(SPACE_ID), /* NOP */
+ 0, /* NOP */
};
#undef bit
diff --git a/src/box/request.c b/src/box/request.c
index fda54a18..6633153c 100644
--- a/src/box/request.c
+++ b/src/box/request.c
@@ -50,7 +50,6 @@ request_create_from_tuple(struct request *request, struct space *space,
struct tuple *old_tuple, struct tuple *new_tuple)
{
memset(request, 0, sizeof(*request));
- request->space_id = space->def->id;
if (old_tuple == new_tuple) {
/*
@@ -61,6 +60,7 @@ request_create_from_tuple(struct request *request, struct space *space,
return 0;
}
+ request->space_id = space->def->id;
if (new_tuple == NULL) {
uint32_t size, key_size;
const char *data = tuple_data_range(old_tuple, &size);
diff --git a/src/box/txn.c b/src/box/txn.c
index e25c0e0e..362030a9 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -71,9 +71,9 @@ txn_add_redo(struct txn_stmt *stmt, struct request *request)
return 0;
}
-/** Initialize a new stmt object within txn. */
+/** Allocate a new txn statement. */
static struct txn_stmt *
-txn_stmt_new(struct txn *txn)
+txn_stmt_new(void)
{
struct txn_stmt *stmt;
stmt = region_alloc_object(&fiber()->gc, struct txn_stmt);
@@ -89,12 +89,6 @@ txn_stmt_new(struct txn *txn)
stmt->new_tuple = NULL;
stmt->engine_savepoint = NULL;
stmt->row = NULL;
-
- /* Set the savepoint for statement rollback. */
- txn->sub_stmt_begin[txn->in_sub_stmt] = stailq_last(&txn->stmts);
- txn->in_sub_stmt++;
-
- stailq_add_tail_entry(&txn->stmts, stmt, next);
return stmt;
}
@@ -106,13 +100,15 @@ txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp)
stailq_cut_tail(&txn->stmts, svp, &rollback);
stailq_reverse(&rollback);
stailq_foreach_entry(stmt, &rollback, next) {
- engine_rollback_statement(txn->engine, txn, stmt);
+ if (stmt->space != NULL) {
+ engine_rollback_statement(txn->engine, txn, stmt);
+ stmt->space = NULL;
+ }
if (stmt->row != NULL) {
assert(txn->n_rows > 0);
txn->n_rows--;
stmt->row = NULL;
}
- stmt->space = NULL;
}
}
@@ -145,7 +141,6 @@ int
txn_begin_in_engine(struct engine *engine, struct txn *txn)
{
if (txn->engine == NULL) {
- assert(stailq_empty(&txn->stmts));
txn->engine = engine;
return engine_begin(engine, txn);
} else if (txn->engine != engine) {
@@ -179,11 +174,17 @@ txn_begin_stmt(struct space *space)
if (txn_begin_in_engine(engine, txn) != 0)
goto fail;
- struct txn_stmt *stmt = txn_stmt_new(txn);
+ struct txn_stmt *stmt = txn_stmt_new();
if (stmt == NULL)
goto fail;
stmt->space = space;
+ /* Set the savepoint for statement rollback. */
+ txn->sub_stmt_begin[txn->in_sub_stmt] = stailq_last(&txn->stmts);
+ txn->in_sub_stmt++;
+
+ stailq_add_tail_entry(&txn->stmts, stmt, next);
+
if (engine_begin_statement(engine, txn) != 0) {
txn_rollback_stmt();
return NULL;
@@ -238,6 +239,35 @@ fail:
return -1;
}
+int
+txn_commit_nop(struct request *request)
+{
+ assert(request->header != NULL);
+
+ struct txn *txn = in_txn();
+ if (txn == NULL) {
+ txn = txn_begin(true);
+ if (txn == NULL)
+ return -1;
+ }
+
+ struct txn_stmt *stmt = txn_stmt_new();
+ if (stmt == NULL)
+ goto fail;
+ if (txn_add_redo(stmt, request) != 0)
+ goto fail;
+
+ txn->n_rows++;
+ stailq_add_tail_entry(&txn->stmts, stmt, next);
+
+ if (txn->is_autocommit && txn->in_sub_stmt == 0)
+ return txn_commit(txn);
+ return 0;
+fail:
+ if (txn->is_autocommit && txn->in_sub_stmt == 0)
+ txn_rollback();
+ return -1;
+}
static int64_t
txn_write_to_wal(struct txn *txn)
@@ -287,32 +317,31 @@ txn_commit(struct txn *txn)
{
assert(txn == in_txn());
- assert(stailq_empty(&txn->stmts) || txn->engine);
-
/* Do transaction conflict resolving */
- if (txn->engine) {
- if (engine_prepare(txn->engine, txn) != 0)
- goto fail;
+ if (txn->engine != NULL &&
+ engine_prepare(txn->engine, txn) != 0)
+ goto fail;
- if (txn->n_rows > 0) {
- txn->signature = txn_write_to_wal(txn);
- if (txn->signature < 0)
- goto fail;
- }
- /*
- * The transaction is in the binary log. No action below
- * may throw. In case an error has happened, there is
- * no other option but terminate.
- */
- if (txn->has_triggers &&
- trigger_run(&txn->on_commit, txn) != 0) {
- diag_log();
- unreachable();
- panic("commit trigger failed");
- }
+ if (txn->n_rows > 0) {
+ txn->signature = txn_write_to_wal(txn);
+ if (txn->signature < 0)
+ goto fail;
+ }
+ /*
+ * The transaction is in the binary log. No action below
+ * may throw. In case an error has happened, there is
+ * no other option but terminate.
+ */
+ if (txn->has_triggers &&
+ trigger_run(&txn->on_commit, txn) != 0) {
+ diag_log();
+ unreachable();
+ panic("commit trigger failed");
+ }
+ if (txn->engine != NULL)
engine_commit(txn->engine, txn);
- }
+
TRASH(txn);
/** Free volatile txn memory. */
fiber_gc();
@@ -470,7 +499,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
}
struct txn_stmt *stmt = svp->stmt == NULL ? NULL :
stailq_entry(svp->stmt, struct txn_stmt, next);
- if (stmt != NULL && stmt->space == NULL) {
+ if (stmt != NULL && stmt->space == NULL && stmt->row == NULL) {
/*
* The statement at which this savepoint was
* created has been rolled back.
diff --git a/src/box/txn.h b/src/box/txn.h
index f3d690be..b61adc33 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -253,6 +253,18 @@ void
txn_rollback_stmt();
/**
+ * Commit a no-op request.
+ *
+ * A no-op request does not affect any space, but it
+ * promotes vclock and is written to WAL.
+ *
+ * This function can be called both as a part of an
+ * independent transaction and in autocommit mode.
+ */
+int
+txn_commit_nop(struct request *request);
+
+/**
* Raise an error if this is a multi-statement
* transaction: DDL can not be part of a multi-statement
* transaction and must be run in autocommit mode.
diff --git a/test/box/before_replace.result b/test/box/before_replace.result
index 4f47b9fa..2b6c1801 100644
--- a/test/box/before_replace.result
+++ b/test/box/before_replace.result
@@ -660,9 +660,9 @@ row.HEADER.type
---
- NOP
...
-row.BODY.space_id == s.id
+#row.BODY
---
-- true
+- 0
...
-- 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 22733c1d..9b4f49cf 100644
--- a/test/box/before_replace.test.lua
+++ b/test/box/before_replace.test.lua
@@ -221,7 +221,7 @@ path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn -
fun, param, state = xlog.pairs(path)
state, row = fun(param, state)
row.HEADER.type
-row.BODY.space_id == s.id
+#row.BODY
-- gh-3128 before_replace with run_triggers
s2 = box.schema.space.create("test2")
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-13 16:10 ` [PATCH 4/6] txn: do not require space id for nop requests Vladimir Davydov
@ 2018-06-27 17:45 ` Konstantin Osipov
2018-06-28 9:13 ` Vladimir Davydov
0 siblings, 1 reply; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-27 17:45 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> Currently, IPROTO_NOP can only be generated by a before_replace trigger,
> when it returns the old tuple thus turning the original operation into a
> NOP. In such a case we know the space id and we write it to the request
> body. This allows us to dispatch NOP requests via DML route.
>
> As a part of replica local spaces feature, we will substitute requests
> operating on local spaces with NOP in relay in order to promote vclock
> on replicas without actual data modification. Since space_id is stored
> in request body, sending it to replicas would mean decoding the request
> body in relay, which is an overkill. To avoid that, let's separate NOP
> and DML paths and remove space_id from NOP requests.
LGTM, a few comments below.
> @@ -277,6 +277,11 @@ apply_row(struct xstream *stream, struct xrow_header *row)
> (void) stream;
> struct request request;
> xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
> + if (request.type == IPROTO_NOP) {
> + if (txn_commit_nop(&request) != 0)
> + diag_raise();
> + return;
> + }
txn_commit_nop() -> box_process_nop() or process_nop().
> @@ -50,7 +50,6 @@ request_create_from_tuple(struct request *request, struct space *space,
> struct tuple *old_tuple, struct tuple *new_tuple)
> {
> memset(request, 0, sizeof(*request));
> - request->space_id = space->def->id;
>
> if (old_tuple == new_tuple) {
> /*
> @@ -61,6 +60,7 @@ request_create_from_tuple(struct request *request, struct space *space,
> return 0;
> }
>
> + request->space_id = space->def->id;
Please add a comment
/*
* Space pointer may be zero in case of NOP, in which case this
* line is not reached.
*/
You changed a lot of code making it more NOP friendly, for example
txn_begin_in_engine() now works even if there is already a
transaction which has a NOP statement. This seems like a bug fix
with no test. Could you please add a test?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-27 17:45 ` Konstantin Osipov
@ 2018-06-28 9:13 ` Vladimir Davydov
2018-06-28 10:23 ` Konstantin Osipov
0 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-28 9:13 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Wed, Jun 27, 2018 at 08:45:09PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> > Currently, IPROTO_NOP can only be generated by a before_replace trigger,
> > when it returns the old tuple thus turning the original operation into a
> > NOP. In such a case we know the space id and we write it to the request
> > body. This allows us to dispatch NOP requests via DML route.
> >
> > As a part of replica local spaces feature, we will substitute requests
> > operating on local spaces with NOP in relay in order to promote vclock
> > on replicas without actual data modification. Since space_id is stored
> > in request body, sending it to replicas would mean decoding the request
> > body in relay, which is an overkill. To avoid that, let's separate NOP
> > and DML paths and remove space_id from NOP requests.
>
> LGTM, a few comments below.
>
> > @@ -277,6 +277,11 @@ apply_row(struct xstream *stream, struct xrow_header *row)
> > (void) stream;
> > struct request request;
> > xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
> > + if (request.type == IPROTO_NOP) {
> > + if (txn_commit_nop(&request) != 0)
> > + diag_raise();
> > + return;
> > + }
>
> txn_commit_nop() -> box_process_nop() or process_nop().
But txn_commit_nop() lives in txn.c. That's why I called it like that.
>
> > @@ -50,7 +50,6 @@ request_create_from_tuple(struct request *request, struct space *space,
> > struct tuple *old_tuple, struct tuple *new_tuple)
> > {
> > memset(request, 0, sizeof(*request));
> > - request->space_id = space->def->id;
> >
> > if (old_tuple == new_tuple) {
> > /*
> > @@ -61,6 +60,7 @@ request_create_from_tuple(struct request *request, struct space *space,
> > return 0;
> > }
> >
> > + request->space_id = space->def->id;
>
> Please add a comment
> /*
> * Space pointer may be zero in case of NOP, in which case this
> * line is not reached.
> */
OK.
>
> You changed a lot of code making it more NOP friendly, for example
> txn_begin_in_engine() now works even if there is already a
> transaction which has a NOP statement. This seems like a bug fix
> with no test.
This is not a bug fix: before this patch there couldn't be a transaction
without engine, because NOP requests also had engine set as they needed
a space.
> Could you please add a test?
The code is already covered by tests. See replication/before_replace,
box/before_replace.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-28 9:13 ` Vladimir Davydov
@ 2018-06-28 10:23 ` Konstantin Osipov
2018-06-28 10:35 ` Vladimir Davydov
0 siblings, 1 reply; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-28 10:23 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 12:14]:
> But txn_commit_nop() lives in txn.c. That's why I called it like that.
Let's move it then.
>
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-28 10:23 ` Konstantin Osipov
@ 2018-06-28 10:35 ` Vladimir Davydov
2018-06-28 10:54 ` Konstantin Osipov
0 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-28 10:35 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jun 28, 2018 at 01:23:22PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 12:14]:
> > But txn_commit_nop() lives in txn.c. That's why I called it like that.
>
> Let's move it then.
That's going to look weird, because this function depends on internal
txn stuff. IMHO it all looks consistent as it is:
txn_begin_stmt - prepare DML request in transaction
txn_commit_stmt - commit DML request statement
txn_commit_nop - commit NOP request statement.
No?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-28 10:35 ` Vladimir Davydov
@ 2018-06-28 10:54 ` Konstantin Osipov
2018-06-28 11:10 ` Vladimir Davydov
0 siblings, 1 reply; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-28 10:54 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 13:39]:
> On Thu, Jun 28, 2018 at 01:23:22PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 12:14]:
> > > But txn_commit_nop() lives in txn.c. That's why I called it like that.
> >
> > Let's move it then.
>
> That's going to look weird, because this function depends on internal
> txn stuff. IMHO it all looks consistent as it is:
>
> txn_begin_stmt - prepare DML request in transaction
> txn_commit_stmt - commit DML request statement
>
> txn_commit_nop - commit NOP request statement.
>
> No?
I see no reason why it should depend on it. Ideally you should be
able to simply call txn_begin_stmt()/txn_commit_stmt(), and that's
it.
What's the problem?
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/6] txn: do not require space id for nop requests
2018-06-28 10:54 ` Konstantin Osipov
@ 2018-06-28 11:10 ` Vladimir Davydov
0 siblings, 0 replies; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-28 11:10 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jun 28, 2018 at 01:54:22PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 13:39]:
> > On Thu, Jun 28, 2018 at 01:23:22PM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/28 12:14]:
> > > > But txn_commit_nop() lives in txn.c. That's why I called it like that.
> > >
> > > Let's move it then.
> >
> > That's going to look weird, because this function depends on internal
> > txn stuff. IMHO it all looks consistent as it is:
> >
> > txn_begin_stmt - prepare DML request in transaction
> > txn_commit_stmt - commit DML request statement
> >
> > txn_commit_nop - commit NOP request statement.
> >
> > No?
>
> I see no reason why it should depend on it. Ideally you should be
> able to simply call txn_begin_stmt()/txn_commit_stmt(), and that's
> it.
>
> What's the problem?
In order to call txn_begin_stmt()/txn_commit_stmt() for a NOP request,
we'll have to teach them to handle space == NULL, but I don't want to
add ifs to the hot path, that's why I decieded to introduce a new
function for handling NOP.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 5/6] xrow: make NOP requests bodiless
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
` (3 preceding siblings ...)
2018-06-13 16:10 ` [PATCH 4/6] txn: do not require space id for nop requests Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-27 17:49 ` Konstantin Osipov
2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov
5 siblings, 1 reply; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
A NOP request has no body, but since it is treated as DML, we still
encode a zero-size map for its body. This complicates conversion of
local requests to NOP in relay as we can't omit xrow_encode_dml (see
the next patch), so let's allow DML requests to be bodiless.
Needed for #3443
---
src/box/box.cc | 1 -
src/box/lua/xlog.c | 15 ++++++++-------
src/box/xrow.c | 19 ++++++++++---------
test/box/before_replace.result | 4 ++--
test/box/before_replace.test.lua | 2 +-
5 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/box/box.cc b/src/box/box.cc
index 8248cc77..7c8e6bf8 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -273,7 +273,6 @@ recovery_journal_create(struct recovery_journal *journal, struct vclock *v)
static inline void
apply_row(struct xstream *stream, struct xrow_header *row)
{
- assert(row->bodycnt == 1); /* always 1 for read */
(void) stream;
struct request request;
xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
diff --git a/src/box/lua/xlog.c b/src/box/lua/xlog.c
index 70384c1d..2271c829 100644
--- a/src/box/lua/xlog.c
+++ b/src/box/lua/xlog.c
@@ -219,13 +219,14 @@ lbox_xlog_parser_iterate(struct lua_State *L)
lua_settable(L, -3); /* HEADER */
- assert(row.bodycnt == 1); /* always 1 for read */
- lua_pushstring(L, "BODY");
- lua_newtable(L);
- lbox_xlog_parse_body(L, row.type, (char *)row.body[0].iov_base,
- row.body[0].iov_len);
- lua_settable(L, -3); /* BODY */
-
+ if (row.bodycnt > 0) {
+ assert(row.bodycnt == 1);
+ lua_pushstring(L, "BODY");
+ lua_newtable(L);
+ lbox_xlog_parse_body(L, row.type, row.body[0].iov_base,
+ row.body[0].iov_len);
+ lua_settable(L, -3); /* BODY */
+ }
return 2;
}
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 48fbff27..64d845f7 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -405,11 +405,12 @@ int
xrow_decode_dml(struct xrow_header *row, struct request *request,
uint64_t key_map)
{
- if (row->bodycnt == 0) {
- diag_set(ClientError, ER_INVALID_MSGPACK,
- "missing request body");
- return -1;
- }
+ memset(request, 0, sizeof(*request));
+ request->header = row;
+ request->type = row->type;
+
+ if (row->bodycnt == 0)
+ goto done;
assert(row->bodycnt == 1);
const char *data = (const char *) row->body[0].iov_base;
@@ -422,10 +423,6 @@ error:
return -1;
}
- memset(request, 0, sizeof(*request));
- request->header = row;
- request->type = row->type;
-
uint32_t size = mp_decode_map(&data);
for (uint32_t i = 0; i < size; i++) {
if (! iproto_dml_body_has_key(data, end)) {
@@ -480,6 +477,7 @@ error:
diag_set(ClientError, ER_INVALID_MSGPACK, "packet end");
return -1;
}
+done:
if (key_map) {
enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
diag_set(ClientError, ER_MISSING_REQUEST_FIELD,
@@ -567,6 +565,9 @@ xrow_encode_dml(const struct request *request, struct iovec *iov)
map_size++;
}
+ if (map_size == 0)
+ return 0;
+
assert(pos <= begin + len);
mp_encode_map(begin, map_size);
iov[0].iov_base = begin;
diff --git a/test/box/before_replace.result b/test/box/before_replace.result
index 2b6c1801..cd0a3be1 100644
--- a/test/box/before_replace.result
+++ b/test/box/before_replace.result
@@ -660,9 +660,9 @@ row.HEADER.type
---
- NOP
...
-#row.BODY
+row.BODY == nil
---
-- 0
+- true
...
-- 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 9b4f49cf..dd464384 100644
--- a/test/box/before_replace.test.lua
+++ b/test/box/before_replace.test.lua
@@ -221,7 +221,7 @@ path = fio.pathjoin(box.cfg.wal_dir, string.format('%020d.xlog', box.info.lsn -
fun, param, state = xlog.pairs(path)
state, row = fun(param, state)
row.HEADER.type
-#row.BODY
+row.BODY == nil
-- gh-3128 before_replace with run_triggers
s2 = box.schema.space.create("test2")
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH 6/6] Introduce replica local spaces
2018-06-13 16:10 [PATCH 0/6] Introduce replica local spaces Vladimir Davydov
` (4 preceding siblings ...)
2018-06-13 16:10 ` [PATCH 5/6] xrow: make NOP requests bodiless Vladimir Davydov
@ 2018-06-13 16:10 ` Vladimir Davydov
2018-06-13 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
` (2 more replies)
5 siblings, 3 replies; 20+ messages in thread
From: Vladimir Davydov @ 2018-06-13 16:10 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
This patch introduces a new space option, is_local, which if specified
on space creation will render all changes done locally to the space
invisible to other replicas. The option can only be set on space
creation and cannot be altered.
Technically, to support this feature, we introduce a new header flag,
IPROTO_IS_LOCAL, which is set for all rows corresponding to replica
local spaces both in xlog and in snap. Relay won't send snapshot rows
that are marked local. As for xlog rows, it will transform them to
IPROTO_NOP so as to promote vclock on the replica without doing any
actual data modification.
The feature is currently supported for memtx spaces only, but it should
be easy to implement it for vinyl spaces as well.
Closes #3443
---
src/box/alter.cc | 4 +
src/box/iproto_constants.c | 4 +-
src/box/iproto_constants.h | 1 +
src/box/lua/schema.lua | 9 +-
src/box/lua/space.cc | 5 ++
src/box/lua/xlog.c | 5 ++
src/box/memtx_engine.c | 8 +-
src/box/relay.cc | 17 +++-
src/box/space.h | 12 ++-
src/box/space_def.c | 2 +
src/box/space_def.h | 5 ++
src/box/txn.c | 3 +
src/box/vinyl.c | 5 ++
src/box/xrow.c | 9 ++
src/box/xrow.h | 1 +
test/engine/iterator.result | 2 +-
test/replication/local_spaces.result | 159 +++++++++++++++++++++++++++++++++
test/replication/local_spaces.test.lua | 54 +++++++++++
test/replication/suite.cfg | 1 +
test/vinyl/ddl.result | 5 ++
test/vinyl/ddl.test.lua | 3 +
21 files changed, 304 insertions(+), 10 deletions(-)
create mode 100644 test/replication/local_spaces.result
create mode 100644 test/replication/local_spaces.test.lua
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 6f6fcb09..9184a284 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1628,6 +1628,10 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
tnt_raise(ClientError, ER_ALTER_SPACE,
space_name(old_space),
"can not change space engine");
+ if (def->opts.local != space_is_local(old_space))
+ tnt_raise(ClientError, ER_ALTER_SPACE,
+ space_name(old_space),
+ "can not switch local flag");
/*
* Allow change of space properties, but do it
* in WAL-error-safe mode.
diff --git a/src/box/iproto_constants.c b/src/box/iproto_constants.c
index 5c1d3a31..38e2bda9 100644
--- a/src/box/iproto_constants.c
+++ b/src/box/iproto_constants.c
@@ -40,10 +40,10 @@ const unsigned char iproto_key_type[IPROTO_KEY_MAX] =
/* 0x04 */ MP_DOUBLE, /* IPROTO_TIMESTAMP */
/* 0x05 */ MP_UINT, /* IPROTO_SCHEMA_VERSION */
/* 0x06 */ MP_UINT, /* IPROTO_SERVER_VERSION */
+ /* 0x07 */ MP_BOOL, /* IPROTO_IS_LOCAL */
/* }}} */
/* {{{ unused */
- /* 0x07 */ MP_UINT,
/* 0x08 */ MP_UINT,
/* 0x09 */ MP_UINT,
/* 0x0a */ MP_UINT,
@@ -133,7 +133,7 @@ const char *iproto_key_strs[IPROTO_KEY_MAX] = {
"timestamp", /* 0x04 */
"schema version", /* 0x05 */
"server version", /* 0x06 */
- NULL, /* 0x07 */
+ "is local", /* 0x07 */
NULL, /* 0x08 */
NULL, /* 0x09 */
NULL, /* 0x0a */
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index d1320de7..445d80d7 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -58,6 +58,7 @@ enum iproto_key {
IPROTO_TIMESTAMP = 0x04,
IPROTO_SCHEMA_VERSION = 0x05,
IPROTO_SERVER_VERSION = 0x06,
+ IPROTO_IS_LOCAL = 0x07,
/* Leave a gap for other keys in the header. */
IPROTO_SPACE_ID = 0x10,
IPROTO_INDEX_ID = 0x11,
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 43c7d4e6..7dd73899 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -419,11 +419,13 @@ box.schema.space.create = function(name, options)
user = 'string, number',
format = 'table',
temporary = 'boolean',
+ is_local = 'boolean',
}
local options_defaults = {
engine = 'memtx',
field_count = 0,
temporary = false,
+ is_local = false,
}
check_param_table(options, options_template)
options = update_param_table(options, options_defaults)
@@ -462,6 +464,7 @@ box.schema.space.create = function(name, options)
-- filter out global parameters from the options array
local space_options = setmap({
temporary = options.temporary and true or nil,
+ is_local = options.is_local and true or nil,
})
_space:insert{id, uid, name, options.engine, options.field_count,
space_options, format}
@@ -2330,7 +2333,11 @@ local function box_space_mt(tab)
for k,v in pairs(tab) do
-- skip system spaces and views
if type(k) == 'string' and #k > 0 and k:sub(1,1) ~= '_' then
- t[k] = { engine = v.engine, temporary = v.temporary }
+ t[k] = {
+ engine = v.engine,
+ temporary = v.temporary,
+ is_local = v.is_local,
+ }
end
end
return t
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 52438275..e59e77bb 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -165,6 +165,11 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
lua_pushboolean(L, space_is_temporary(space));
lua_settable(L, i);
+ /* space.is_local */
+ lua_pushstring(L, "is_local");
+ lua_pushboolean(L, space_is_local(space));
+ lua_settable(L, i);
+
/* space.name */
lua_pushstring(L, "name");
lua_pushstring(L, space_name(space));
diff --git a/src/box/lua/xlog.c b/src/box/lua/xlog.c
index 2271c829..5e270f16 100644
--- a/src/box/lua/xlog.c
+++ b/src/box/lua/xlog.c
@@ -211,6 +211,11 @@ lbox_xlog_parser_iterate(struct lua_State *L)
lua_pushinteger(L, row.replica_id);
lua_settable(L, -3); /* replica_id */
}
+ if (row.is_local) {
+ lbox_xlog_pushkey(L, iproto_key_name(IPROTO_IS_LOCAL));
+ lua_pushboolean(L, row.is_local);
+ lua_settable(L, -3); /* is_local */
+ }
if (row.tm != 0) {
lbox_xlog_pushkey(L, iproto_key_name(IPROTO_TIMESTAMP));
lua_pushnumber(L, row.tm);
diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index de9fd1ba..60959402 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -492,19 +492,20 @@ checkpoint_write_row(struct xlog *l, struct xrow_header *row)
}
static int
-checkpoint_write_tuple(struct xlog *l, uint32_t space_id,
+checkpoint_write_tuple(struct xlog *l, struct space *space,
const char *data, uint32_t size)
{
struct request_replace_body body;
body.m_body = 0x82; /* map of two elements. */
body.k_space_id = IPROTO_SPACE_ID;
body.m_space_id = 0xce; /* uint32 */
- body.v_space_id = mp_bswap_u32(space_id);
+ body.v_space_id = mp_bswap_u32(space_id(space));
body.k_tuple = IPROTO_TUPLE;
struct xrow_header row;
memset(&row, 0, sizeof(struct xrow_header));
row.type = IPROTO_INSERT;
+ row.is_local = space_is_local(space);
row.bodycnt = 2;
row.body[0].iov_base = &body;
@@ -629,8 +630,7 @@ checkpoint_f(va_list ap)
struct snapshot_iterator *it = entry->iterator;
for (data = it->next(it, &size); data != NULL;
data = it->next(it, &size)) {
- if (checkpoint_write_tuple(&snap,
- space_id(entry->space),
+ if (checkpoint_write_tuple(&snap, entry->space,
data, size) != 0) {
xlog_close(&snap, false);
return -1;
diff --git a/src/box/relay.cc b/src/box/relay.cc
index a25cc540..322266fc 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -620,7 +620,12 @@ static void
relay_send_initial_join_row(struct xstream *stream, struct xrow_header *row)
{
struct relay *relay = container_of(stream, struct relay, stream);
- relay_send(relay, row);
+ /*
+ * Ignore replica local requests as we don't need to promote
+ * vclock while sending a snapshot.
+ */
+ if (!row->is_local)
+ relay_send(relay, row);
}
/** Send a single row to the client. */
@@ -630,6 +635,16 @@ relay_send_row(struct xstream *stream, struct xrow_header *packet)
struct relay *relay = container_of(stream, struct relay, stream);
assert(iproto_type_is_dml(packet->type));
/*
+ * Transform replica local requests to IPROTO_NOP so as to
+ * promote vclock on the replica without actually modifying
+ * any data.
+ */
+ if (packet->is_local) {
+ packet->type = IPROTO_NOP;
+ packet->is_local = false;
+ packet->bodycnt = 0;
+ }
+ /*
* We're feeding a WAL, thus responding to SUBSCRIBE request.
* In that case, only send a row if it is not from the same replica
* (i.e. don't send replica's own rows back) or if this row is
diff --git a/src/box/space.h b/src/box/space.h
index a024fdc8..2a852ed3 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -193,7 +193,17 @@ space_name(const struct space *space)
/** Return true if space is temporary. */
static inline bool
-space_is_temporary(struct space *space) { return space->def->opts.temporary; }
+space_is_temporary(struct space *space)
+{
+ return space->def->opts.temporary;
+}
+
+/** Return true if space is replica local. */
+static inline bool
+space_is_local(struct space *space)
+{
+ return space->def->opts.local;
+}
void
space_run_triggers(struct space *space, bool yesno);
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 7349c214..0b6a6d59 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -35,10 +35,12 @@
const struct space_opts space_opts_default = {
/* .temporary = */ false,
+ /* .local = */ false,
};
const struct opt_def space_opts_reg[] = {
OPT_DEF("temporary", OPT_BOOL, struct space_opts, temporary),
+ OPT_DEF("is_local", OPT_BOOL, struct space_opts, local),
OPT_END,
};
diff --git a/src/box/space_def.h b/src/box/space_def.h
index 97c7e138..26e60d03 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -48,6 +48,11 @@ struct space_opts {
* - changes are not part of a snapshot
*/
bool temporary;
+ /**
+ * The space is replica local: changes
+ * done to it are not replicated.
+ */
+ bool local;
};
extern const struct space_opts space_opts_default;
diff --git a/src/box/txn.c b/src/box/txn.c
index 362030a9..82248e34 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -64,6 +64,7 @@ txn_add_redo(struct txn_stmt *stmt, struct request *request)
row->lsn = 0;
row->sync = 0;
row->tm = 0;
+ row->is_local = false;
row->bodycnt = xrow_encode_dml(request, row->body);
if (row->bodycnt < 0)
return -1;
@@ -214,6 +215,8 @@ txn_commit_stmt(struct txn *txn, struct request *request)
if (!space_is_temporary(stmt->space)) {
if (txn_add_redo(stmt, request) != 0)
goto fail;
+ if (space_is_local(stmt->space))
+ stmt->row->is_local = true;
++txn->n_rows;
}
/*
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index d2e3da7e..d99fe220 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -595,6 +595,11 @@ vinyl_engine_check_space_def(struct space_def *def)
def->name, "engine does not support temporary flag");
return -1;
}
+ if (def->opts.local) {
+ diag_set(ClientError, ER_ALTER_SPACE,
+ def->name, "engine does not support local flag");
+ return -1;
+ }
return 0;
}
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 64d845f7..e5d8d045 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -106,6 +106,9 @@ error:
case IPROTO_SCHEMA_VERSION:
header->schema_version = mp_decode_uint(pos);
break;
+ case IPROTO_IS_LOCAL:
+ header->is_local = mp_decode_bool(pos);
+ break;
default:
/* unknown header */
mp_next(pos);
@@ -178,6 +181,12 @@ xrow_header_encode(const struct xrow_header *header, uint64_t sync,
map_size++;
}
+ if (header->is_local) {
+ d = mp_encode_uint(d, IPROTO_IS_LOCAL);
+ d = mp_encode_bool(d, header->is_local);
+ map_size++;
+ }
+
if (header->lsn) {
d = mp_encode_uint(d, IPROTO_LSN);
d = mp_encode_uint(d, header->lsn);
diff --git a/src/box/xrow.h b/src/box/xrow.h
index 1bb5f103..36211220 100644
--- a/src/box/xrow.h
+++ b/src/box/xrow.h
@@ -60,6 +60,7 @@ struct xrow_header {
uint64_t sync;
int64_t lsn; /* LSN must be signed for correct comparison */
double tm;
+ bool is_local; /* set for replica-local records */
int bodycnt;
uint32_t schema_version;
diff --git a/test/engine/iterator.result b/test/engine/iterator.result
index 1bde10ea..91ad325f 100644
--- a/test/engine/iterator.result
+++ b/test/engine/iterator.result
@@ -4211,7 +4211,7 @@ s:replace{35}
...
state, value = gen(param,state)
---
-- error: 'builtin/box/schema.lua:1032: usage: next(param, state)'
+- error: 'builtin/box/schema.lua:1035: usage: next(param, state)'
...
value
---
diff --git a/test/replication/local_spaces.result b/test/replication/local_spaces.result
new file mode 100644
index 00000000..2ef74edd
--- /dev/null
+++ b/test/replication/local_spaces.result
@@ -0,0 +1,159 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+--
+-- gh-3443: Check that changes done to spaces marked as local
+-- are not replicated, but vclock is still promoted.
+--
+s1 = box.schema.space.create('test1')
+---
+...
+_ = s1:create_index('pk')
+---
+...
+s2 = box.schema.space.create('test2', {is_local = true})
+---
+...
+_ = s2:create_index('pk')
+---
+...
+s1.is_local
+---
+- false
+...
+s2.is_local
+---
+- true
+...
+_ = s1:insert{1}
+---
+...
+_ = s2:insert{1}
+---
+...
+box.snapshot()
+---
+- ok
+...
+_ = s1:insert{2}
+---
+...
+_ = s2:insert{2}
+---
+...
+box.schema.user.grant('guest', 'replication')
+---
+...
+test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")
+---
+- true
+...
+test_run:cmd("start server replica")
+---
+- true
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+box.space.test1.is_local
+---
+- false
+...
+box.space.test2.is_local
+---
+- true
+...
+box.space.test1:select()
+---
+- - [1]
+ - [2]
+...
+box.space.test2:select()
+---
+- []
+...
+for i = 1, 3 do box.space.test2:insert{i, i} end
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+_ = s1:insert{3}
+---
+...
+_ = s2:insert{3}
+---
+...
+vclock = test_run:get_vclock('default')
+---
+...
+_ = test_run:wait_vclock('replica', vclock)
+---
+...
+test_run:cmd("switch replica")
+---
+- true
+...
+box.space.test1:select()
+---
+- - [1]
+ - [2]
+ - [3]
+...
+box.space.test2:select()
+---
+- - [1, 1]
+ - [2, 2]
+ - [3, 3]
+...
+test_run:cmd("restart server replica")
+box.space.test1:select()
+---
+- - [1]
+ - [2]
+ - [3]
+...
+box.space.test2:select()
+---
+- - [1, 1]
+ - [2, 2]
+ - [3, 3]
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd("stop server replica")
+---
+- true
+...
+test_run:cmd("cleanup server replica")
+---
+- true
+...
+box.schema.user.revoke('guest', 'replication')
+---
+...
+s1:select()
+---
+- - [1]
+ - [2]
+ - [3]
+...
+s2:select()
+---
+- - [1]
+ - [2]
+ - [3]
+...
+s1:drop()
+---
+...
+s2:drop()
+---
+...
diff --git a/test/replication/local_spaces.test.lua b/test/replication/local_spaces.test.lua
new file mode 100644
index 00000000..804c507b
--- /dev/null
+++ b/test/replication/local_spaces.test.lua
@@ -0,0 +1,54 @@
+env = require('test_run')
+test_run = env.new()
+
+--
+-- gh-3443: Check that changes done to spaces marked as local
+-- are not replicated, but vclock is still promoted.
+--
+
+s1 = box.schema.space.create('test1')
+_ = s1:create_index('pk')
+s2 = box.schema.space.create('test2', {is_local = true})
+_ = s2:create_index('pk')
+s1.is_local
+s2.is_local
+_ = s1:insert{1}
+_ = s2:insert{1}
+box.snapshot()
+_ = s1:insert{2}
+_ = s2:insert{2}
+
+box.schema.user.grant('guest', 'replication')
+test_run:cmd("create server replica with rpl_master=default, script='replication/replica.lua'")
+test_run:cmd("start server replica")
+
+test_run:cmd("switch replica")
+box.space.test1.is_local
+box.space.test2.is_local
+box.space.test1:select()
+box.space.test2:select()
+for i = 1, 3 do box.space.test2:insert{i, i} end
+
+test_run:cmd("switch default")
+_ = s1:insert{3}
+_ = s2:insert{3}
+vclock = test_run:get_vclock('default')
+_ = test_run:wait_vclock('replica', vclock)
+
+test_run:cmd("switch replica")
+box.space.test1:select()
+box.space.test2:select()
+test_run:cmd("restart server replica")
+box.space.test1:select()
+box.space.test2:select()
+
+test_run:cmd("switch default")
+test_run:cmd("stop server replica")
+test_run:cmd("cleanup server replica")
+box.schema.user.revoke('guest', 'replication')
+
+s1:select()
+s2:select()
+
+s1:drop()
+s2:drop()
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index 95e94e5a..283edcad 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -6,6 +6,7 @@
"wal_off.test.lua": {},
"hot_standby.test.lua": {},
"rebootstrap.test.lua": {},
+ "local_spaces.test.lua": {},
"*": {
"memtx": {"engine": "memtx"},
"vinyl": {"engine": "vinyl"}
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 16ee7097..c9498adb 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -44,6 +44,11 @@ space:create_index('pk', {bloom_fpr = 1.1})
space:drop()
---
...
+-- vinyl does not support replica local spaces
+space = box.schema.space.create('test', {engine = 'vinyl', is_local = true})
+---
+- error: 'Can''t modify space ''test'': engine does not support local flag'
+...
-- space secondary index create
space = box.schema.space.create('test', { engine = 'vinyl' })
---
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index 95dd5a11..a02622fc 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -12,6 +12,9 @@ space:create_index('pk', {bloom_fpr = 0})
space:create_index('pk', {bloom_fpr = 1.1})
space:drop()
+-- vinyl does not support replica local spaces
+space = box.schema.space.create('test', {engine = 'vinyl', is_local = true})
+
-- space secondary index create
space = box.schema.space.create('test', { engine = 'vinyl' })
index1 = space:create_index('primary')
--
2.11.0
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [tarantool-patches] [PATCH 6/6] Introduce replica local spaces
2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov
@ 2018-06-13 21:26 ` Vladislav Shpilevoy
2018-06-27 18:24 ` Konstantin Osipov
2018-06-27 18:27 ` Konstantin Osipov
2 siblings, 0 replies; 20+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-13 21:26 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov, kostja
On 13/06/2018 19:10, Vladimir Davydov wrote:
> This patch introduces a new space option, is_local, which if specified
> on space creation will render all changes done locally to the space
> invisible to other replicas. The option can only be set on space
> creation and cannot be altered.
>
> Technically, to support this feature, we introduce a new header flag,
> IPROTO_IS_LOCAL, which is set for all rows corresponding to replica
> local spaces both in xlog and in snap. Relay won't send snapshot rows
> that are marked local. As for xlog rows, it will transform them to
> IPROTO_NOP so as to promote vclock on the replica without doing any
> actual data modification.
>
> The feature is currently supported for memtx spaces only, but it should
> be easy to implement it for vinyl spaces as well.
>
> Closes #3443
Please, write here a documentation bot request. Yes, you can do
it right in a commit message at the end. Using the same syntax
as for tickets.
Once the patch is pushed into the trunk (now it is 2.0), the bot
will create the documentation issue automatically.
After the patch was pushed on any branch, you could check that the
bot got your request here: http://try.tarantool.org:11116 in
TarantoolBot Journal.
> ---
> src/box/alter.cc | 4 +
> src/box/iproto_constants.c | 4 +-
> src/box/iproto_constants.h | 1 +
> src/box/lua/schema.lua | 9 +-
> src/box/lua/space.cc | 5 ++
> src/box/lua/xlog.c | 5 ++
> src/box/memtx_engine.c | 8 +-
> src/box/relay.cc | 17 +++-
> src/box/space.h | 12 ++-
> src/box/space_def.c | 2 +
> src/box/space_def.h | 5 ++
> src/box/txn.c | 3 +
> src/box/vinyl.c | 5 ++
> src/box/xrow.c | 9 ++
> src/box/xrow.h | 1 +
> test/engine/iterator.result | 2 +-
> test/replication/local_spaces.result | 159 +++++++++++++++++++++++++++++++++
> test/replication/local_spaces.test.lua | 54 +++++++++++
> test/replication/suite.cfg | 1 +
> test/vinyl/ddl.result | 5 ++
> test/vinyl/ddl.test.lua | 3 +
> 21 files changed, 304 insertions(+), 10 deletions(-)
> create mode 100644 test/replication/local_spaces.result
> create mode 100644 test/replication/local_spaces.test.lua
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] Introduce replica local spaces
2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov
2018-06-13 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-27 18:24 ` Konstantin Osipov
2018-06-27 18:27 ` Konstantin Osipov
2 siblings, 0 replies; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-27 18:24 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> This patch introduces a new space option, is_local, which if specified
> on space creation will render all changes done locally to the space
> invisible to other replicas. The option can only be set on space
> creation and cannot be altered.
>
> Technically, to support this feature, we introduce a new header flag,
> IPROTO_IS_LOCAL, which is set for all rows corresponding to replica
> local spaces both in xlog and in snap. Relay won't send snapshot rows
> that are marked local. As for xlog rows, it will transform them to
> IPROTO_NOP so as to promote vclock on the replica without doing any
> actual data modification.
>
> The feature is currently supported for memtx spaces only, but it should
> be easy to implement it for vinyl spaces as well.
The patch overall LGTM, except is_local in xrow should be a number
instead for replication group.
The following group identifiers should be reserved:
0 - the default replication group, entire cluster
1 - local space.
2 - shard-local space.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/6] Introduce replica local spaces
2018-06-13 16:10 ` [PATCH 6/6] Introduce replica local spaces Vladimir Davydov
2018-06-13 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-27 18:24 ` Konstantin Osipov
@ 2018-06-27 18:27 ` Konstantin Osipov
2 siblings, 0 replies; 20+ messages in thread
From: Konstantin Osipov @ 2018-06-27 18:27 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [18/06/13 20:40]:
> @@ -48,6 +48,11 @@ struct space_opts {
> * - changes are not part of a snapshot
> */
> bool temporary;
> + /**
> + * The space is replica local: changes
> + * done to it are not replicated.
> + */
> + bool local;
Argh, I hate this violation of the coding style. There is bool
view in 2.0. Please make these flags bits to save space and prefix
them all with is_ once and for all.
Finally, the patch should contain a petition to @docbot so that
there is a documentation request for it
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 20+ messages in thread