<div dir="auto">Lgtm</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 21, 2019, 15:16 Vladimir Davydov <<a href="mailto:vdavydov.dev@gmail.com">vdavydov.dev@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Certain kinds of DML requests don't update secondary indexes, e.g.<br>
UPDATE that doesn't touch secondary index parts or DELETE for which<br>
generation of secondary index statements is deferred. For such a request<br>
vy_is_committed(env, space) may return false on recovery even if it has<br>
actually been dumped: since such a statement is not dumped for secondary<br>
indexes, secondary index's vy_lsm::dump_lsn may be less than such<br>
statement's signature, which makes vy_is_committed() assume that the<br>
statement hasn't been dumped. Further in the code we have checks that<br>
ensure that if we execute a request on recovery, it must not be dumped<br>
for the primary index (as the primary index is always dumped after<br>
secondary indexes for the sake of recovery), which fires in this case.<br>
<br>
To fix that, let's refactor the code basing on the following two facts:<br>
- Primary index is always updated by a DML request.<br>
- Primary index may only be dumped after secondary indexes.<br>
<br>
Closes #4222<br>
---<br>
<a href="https://github.com/tarantool/tarantool/issues/4222" rel="noreferrer noreferrer" target="_blank">https://github.com/tarantool/tarantool/issues/4222</a><br>
<br>
src/box/vinyl.c | 89 +++++++++++++++++++--------------------------<br>
test/vinyl/recover.result | 60 ++++++++++++++++++++++++++++++<br>
test/vinyl/recover.test.lua | 27 ++++++++++++++<br>
3 files changed, 125 insertions(+), 51 deletions(-)<br>
<br>
diff --git a/src/box/vinyl.c b/src/box/vinyl.c<br>
index d4929a37..9372e5f7 100644<br>
--- a/src/box/vinyl.c<br>
+++ b/src/box/vinyl.c<br>
@@ -1277,9 +1277,17 @@ vinyl_index_compact(struct index *index)<br>
* If the LSM tree is going to be dropped or truncated on WAL<br>
* recovery, there's no point in replaying statements for it,<br>
* either.<br>
+ *<br>
+ * Note, although we may skip secondary index update in certain<br>
+ * cases (e.g. UPDATE that doesn't touch secondary index parts<br>
+ * or DELETE for which generation of secondary index statement<br>
+ * is deferred), a DML request of any kind always updates the<br>
+ * primary index. Also, we always dump the primary index after<br>
+ * secondary indexes. So we may skip recovery of a DML request<br>
+ * if it has been dumped for the primary index.<br>
*/<br>
static inline bool<br>
-vy_is_committed_one(struct vy_env *env, struct vy_lsm *lsm)<br>
+vy_is_committed(struct vy_env *env, struct vy_lsm *lsm)<br>
{<br>
if (likely(env->status != VINYL_FINAL_RECOVERY_LOCAL))<br>
return false;<br>
@@ -1291,23 +1299,6 @@ vy_is_committed_one(struct vy_env *env, struct vy_lsm *lsm)<br>
}<br>
<br>
/**<br>
- * Check if a request has already been committed to a space.<br>
- * See also vy_is_committed_one().<br>
- */<br>
-static inline bool<br>
-vy_is_committed(struct vy_env *env, struct space *space)<br>
-{<br>
- if (likely(env->status != VINYL_FINAL_RECOVERY_LOCAL))<br>
- return false;<br>
- for (uint32_t iid = 0; iid < space->index_count; iid++) {<br>
- struct vy_lsm *lsm = vy_lsm(space->index[iid]);<br>
- if (!vy_is_committed_one(env, lsm))<br>
- return false;<br>
- }<br>
- return true;<br>
-}<br>
-<br>
-/**<br>
* Get a full tuple by a tuple read from a secondary index.<br>
* @param lsm LSM tree from which the tuple was read.<br>
* @param tx Current transaction.<br>
@@ -1772,11 +1763,11 @@ static int<br>
vy_delete(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
struct space *space, struct request *request)<br>
{<br>
- if (vy_is_committed(env, space))<br>
- return 0;<br>
struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
if (pk == NULL)<br>
return -1;<br>
+ if (vy_is_committed(env, pk))<br>
+ return 0;<br>
struct vy_lsm *lsm = vy_lsm_find_unique(space, request->index_id);<br>
if (lsm == NULL)<br>
return -1;<br>
@@ -1807,7 +1798,7 @@ vy_delete(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
return -1;<br>
for (uint32_t i = 0; i < space->index_count; i++) {<br>
struct vy_lsm *lsm = vy_lsm(space->index[i]);<br>
- if (vy_is_committed_one(env, lsm))<br>
+ if (vy_is_committed(env, lsm))<br>
continue;<br>
rc = vy_tx_set(tx, lsm, delete);<br>
if (rc != 0)<br>
@@ -1891,7 +1882,7 @@ vy_perform_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
<br>
for (uint32_t i = 1; i < space->index_count; ++i) {<br>
struct vy_lsm *lsm = vy_lsm(space->index[i]);<br>
- if (vy_is_committed_one(env, lsm))<br>
+ if (vy_is_committed(env, lsm))<br>
continue;<br>
if (vy_tx_set_with_colmask(tx, lsm, delete, column_mask) != 0)<br>
goto error;<br>
@@ -1924,7 +1915,10 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
struct space *space, struct request *request)<br>
{<br>
assert(tx != NULL && tx->state == VINYL_TX_READY);<br>
- if (vy_is_committed(env, space))<br>
+ struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
+ if (pk == NULL)<br>
+ return -1;<br>
+ if (vy_is_committed(env, pk))<br>
return 0;<br>
struct vy_lsm *lsm = vy_lsm_find_unique(space, request->index_id);<br>
if (lsm == NULL)<br>
@@ -1942,11 +1936,6 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
return 0;<br>
<br>
/* Apply update operations. */<br>
- struct vy_lsm *pk = vy_lsm(space->index[0]);<br>
- assert(pk != NULL);<br>
- assert(pk->index_id == 0);<br>
- /* Primary key is dumped last. */<br>
- assert(!vy_is_committed_one(env, pk));<br>
uint64_t column_mask = 0;<br>
const char *new_tuple, *new_tuple_end;<br>
uint32_t new_size, old_size;<br>
@@ -2126,7 +2115,10 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
struct space *space, struct request *request)<br>
{<br>
assert(tx != NULL && tx->state == VINYL_TX_READY);<br>
- if (vy_is_committed(env, space))<br>
+ struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
+ if (pk == NULL)<br>
+ return -1;<br>
+ if (vy_is_committed(env, pk))<br>
return 0;<br>
/* Check update operations. */<br>
if (tuple_update_check_ops(region_aligned_alloc_cb, &fiber()->gc,<br>
@@ -2143,11 +2135,6 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
const char *tuple_end = request->tuple_end;<br>
const char *ops = request->ops;<br>
const char *ops_end = request->ops_end;<br>
- struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
- if (pk == NULL)<br>
- return -1;<br>
- /* Primary key is dumped last. */<br>
- assert(!vy_is_committed_one(env, pk));<br>
if (tuple_validate_raw(pk->mem_format, tuple))<br>
return -1;<br>
<br>
@@ -2240,14 +2227,12 @@ static int<br>
vy_insert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
struct space *space, struct request *request)<br>
{<br>
- assert(stmt != NULL);<br>
+ assert(tx != NULL && tx->state == VINYL_TX_READY);<br>
struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
if (pk == NULL)<br>
- /* The space hasn't the primary index. */<br>
return -1;<br>
- assert(pk->index_id == 0);<br>
- /* Primary key is dumped last. */<br>
- assert(!vy_is_committed_one(env, pk));<br>
+ if (vy_is_committed(env, pk))<br>
+ return 0;<br>
if (tuple_validate_raw(pk->mem_format, request->tuple))<br>
return -1;<br>
/* First insert into the primary index. */<br>
@@ -2263,7 +2248,7 @@ vy_insert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
<br>
for (uint32_t iid = 1; iid < space->index_count; ++iid) {<br>
struct vy_lsm *lsm = vy_lsm(space->index[iid]);<br>
- if (vy_is_committed_one(env, lsm))<br>
+ if (vy_is_committed(env, lsm))<br>
continue;<br>
if (vy_tx_set(tx, lsm, stmt->new_tuple) != 0)<br>
return -1;<br>
@@ -2290,16 +2275,11 @@ vy_replace(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
struct space *space, struct request *request)<br>
{<br>
assert(tx != NULL && tx->state == VINYL_TX_READY);<br>
- if (vy_is_committed(env, space))<br>
- return 0;<br>
- if (request->type == IPROTO_INSERT)<br>
- return vy_insert(env, tx, stmt, space, request);<br>
-<br>
struct vy_lsm *pk = vy_lsm_find(space, 0);<br>
if (pk == NULL)<br>
return -1;<br>
- /* Primary key is dumped last. */<br>
- assert(!vy_is_committed_one(env, pk));<br>
+ if (vy_is_committed(env, pk))<br>
+ return 0;<br>
<br>
/* Validate and create a statement for the new tuple. */<br>
if (tuple_validate_raw(pk->mem_format, request->tuple))<br>
@@ -2352,7 +2332,7 @@ vy_replace(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,<br>
}<br>
for (uint32_t i = 1; i < space->index_count; i++) {<br>
struct vy_lsm *lsm = vy_lsm(space->index[i]);<br>
- if (vy_is_committed_one(env, lsm))<br>
+ if (vy_is_committed(env, lsm))<br>
continue;<br>
if (delete != NULL) {<br>
rc = vy_tx_set(tx, lsm, delete);<br>
@@ -2376,7 +2356,12 @@ vinyl_space_execute_replace(struct space *space, struct txn *txn,<br>
struct vy_env *env = vy_env(space->engine);<br>
struct vy_tx *tx = txn->engine_tx;<br>
struct txn_stmt *stmt = txn_current_stmt(txn);<br>
- if (vy_replace(env, tx, stmt, space, request))<br>
+ int rc;<br>
+ if (request->type == IPROTO_INSERT)<br>
+ rc = vy_insert(env, tx, stmt, space, request);<br>
+ else<br>
+ rc = vy_replace(env, tx, stmt, space, request);<br>
+ if (rc != 0)<br>
return -1;<br>
*result = stmt->new_tuple;<br>
return 0;<br>
@@ -3300,6 +3285,8 @@ vinyl_space_apply_initial_join_row(struct space *space, struct request *request)<br>
int rc = -1;<br>
switch (request->type) {<br>
case IPROTO_INSERT:<br>
+ rc = vy_insert(env, tx, &stmt, space, request);<br>
+ break;<br>
case IPROTO_REPLACE:<br>
rc = vy_replace(env, tx, &stmt, space, request);<br>
break;<br>
@@ -4488,7 +4475,7 @@ vy_deferred_delete_on_rollback(struct trigger *trigger, void *event)<br>
* to restore deferred DELETE statements that haven't been dumped<br>
* to disk. To skip deferred DELETEs that have been dumped, we<br>
* use the same technique we employ for normal WAL statements,<br>
- * i.e. we filter them by LSN, see vy_is_committed_one(). To do<br>
+ * i.e. we filter them by LSN, see vy_is_committed(). To do<br>
* that, we need to account the LSN of a WAL row that generated<br>
* a deferred DELETE statement to vy_lsm::dump_lsn, so we install<br>
* an on_commit trigger that propagates the LSN of the WAL row to<br>
@@ -4575,7 +4562,7 @@ vy_deferred_delete_on_replace(struct trigger *trigger, void *event)<br>
struct tuple *region_stmt = NULL;<br>
for (uint32_t i = 1; i < space->index_count; i++) {<br>
struct vy_lsm *lsm = vy_lsm(space->index[i]);<br>
- if (vy_is_committed_one(env, lsm))<br>
+ if (vy_is_committed(env, lsm))<br>
continue;<br>
/*<br>
* As usual, rotate the active in-memory index if<br>
diff --git a/test/vinyl/recover.result b/test/vinyl/recover.result<br>
index a6313f22..345a019d 100644<br>
--- a/test/vinyl/recover.result<br>
+++ b/test/vinyl/recover.result<br>
@@ -479,3 +479,63 @@ test_run:cmd('cleanup server force_recovery')<br>
---<br>
- true<br>
...<br>
+--<br>
+-- gh-4222: assertion failure while recovering dumped statement<br>
+-- that isn't present in secondary index.<br>
+--<br>
+test_run:cmd('create server test with script = "vinyl/low_quota.lua"')<br>
+---<br>
+- true<br>
+...<br>
+test_run:cmd('start server test with args="1048576"')<br>
+---<br>
+- true<br>
+...<br>
+test_run:cmd('switch test')<br>
+---<br>
+- true<br>
+...<br>
+s = box.schema.space.create('test', {engine = 'vinyl'})<br>
+---<br>
+...<br>
+pk = s:create_index('primary')<br>
+---<br>
+...<br>
+sk = s:create_index('secondary', {unique = false, parts = {2, 'unsigned'}})<br>
+---<br>
+...<br>
+s:insert{1, 1, 1}<br>
+---<br>
+- [1, 1, 1]<br>
+...<br>
+box.snapshot()<br>
+---<br>
+- ok<br>
+...<br>
+for i = 1, 1024 do s:update(1, {{'=', 3, string.rep('x', 1024)}}) end<br>
+---<br>
+...<br>
+test_run:wait_cond(function() return pk:stat().disk.dump.count == 2 end)<br>
+---<br>
+- true<br>
+...<br>
+sk:stat().disk.dump.count -- 1<br>
+---<br>
+- 1<br>
+...<br>
+test_run:cmd('restart server test with args="1048576"')<br>
+box.space.test:drop()<br>
+---<br>
+...<br>
+test_run:cmd('switch default')<br>
+---<br>
+- true<br>
+...<br>
+test_run:cmd('stop server test')<br>
+---<br>
+- true<br>
+...<br>
+test_run:cmd('cleanup server test')<br>
+---<br>
+- true<br>
+...<br>
diff --git a/test/vinyl/recover.test.lua b/test/vinyl/recover.test.lua<br>
index 1ed266a1..7bdba178 100644<br>
--- a/test/vinyl/recover.test.lua<br>
+++ b/test/vinyl/recover.test.lua<br>
@@ -164,3 +164,30 @@ sum<br>
test_run:cmd('switch default')<br>
test_run:cmd('stop server force_recovery')<br>
test_run:cmd('cleanup server force_recovery')<br>
+<br>
+--<br>
+-- gh-4222: assertion failure while recovering dumped statement<br>
+-- that isn't present in secondary index.<br>
+--<br>
+test_run:cmd('create server test with script = "vinyl/low_quota.lua"')<br>
+test_run:cmd('start server test with args="1048576"')<br>
+test_run:cmd('switch test')<br>
+<br>
+s = box.schema.space.create('test', {engine = 'vinyl'})<br>
+pk = s:create_index('primary')<br>
+sk = s:create_index('secondary', {unique = false, parts = {2, 'unsigned'}})<br>
+<br>
+s:insert{1, 1, 1}<br>
+box.snapshot()<br>
+<br>
+for i = 1, 1024 do s:update(1, {{'=', 3, string.rep('x', 1024)}}) end<br>
+test_run:wait_cond(function() return pk:stat().disk.dump.count == 2 end)<br>
+sk:stat().disk.dump.count -- 1<br>
+<br>
+test_run:cmd('restart server test with args="1048576"')<br>
+<br>
+box.space.test:drop()<br>
+<br>
+test_run:cmd('switch default')<br>
+test_run:cmd('stop server test')<br>
+test_run:cmd('cleanup server test')<br>
-- <br>
2.11.0<br>
<br>
<br>
</blockquote></div>