diff --git a/changelogs/unreleased/gh-10751-fix-vinyl-memory-stmt-accounting.md b/changelogs/unreleased/gh-10751-fix-vinyl-memory-stmt-accounting.md new file mode 100644 index 0000000000000000000000000000000000000000..830bb55940e4d01297c5c84ef0b09f2f88e17234 --- /dev/null +++ b/changelogs/unreleased/gh-10751-fix-vinyl-memory-stmt-accounting.md @@ -0,0 +1,5 @@ +## bugfix/vinyl + +* Fixed a bug when `index.stat()` and `index.len()` could report a wrong number + of in-memory statements for a non-unique multi-key index of a space with + the `defer_deletes` option enabled (gh-10751). diff --git a/src/box/vinyl.c b/src/box/vinyl.c index ebe59d4c0a5fc830688d3be5f485a2b395970a21..018f2a409b9845ea3a1d5ef3598180313f46fe0d 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -4039,11 +4039,9 @@ vy_build_insert_stmt(struct vy_lsm *lsm, struct vy_mem *mem, vy_stmt_set_lsn(region_stmt, lsn); struct vy_entry entry; vy_stmt_foreach_entry(entry, region_stmt, lsm->cmp_def) { - if (vy_mem_insert(mem, entry) != 0) + if (vy_mem_insert(mem, entry, &lsm->stat.memory.count) != 0) return -1; vy_mem_commit_stmt(mem, entry); - vy_stmt_counter_acct_tuple(&lsm->stat.memory.count, - region_stmt); } return 0; } diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c index 24aa50b52d05b96a6c784304d18bc8c5b1205916..f4468aed62057205c56579756c454aa69ca7e9a7 100644 --- a/src/box/vy_lsm.c +++ b/src/box/vy_lsm.c @@ -906,6 +906,12 @@ vy_lsm_set(struct vy_lsm *lsm, struct vy_mem *mem, assert(vy_stmt_is_refable(entry.stmt)); assert(*region_stmt == NULL || !vy_stmt_is_refable(*region_stmt)); + /* Abort transaction if format was changed by DDL */ + if (!vy_stmt_is_key(entry.stmt) && + format_id != tuple_format_id(mem->format)) { + diag_set(ClientError, ER_TRANSACTION_CONFLICT); + return -1; + } /* * Allocate region_stmt on demand. * @@ -923,19 +929,11 @@ vy_lsm_set(struct vy_lsm *lsm, struct vy_mem *mem, } entry.stmt = *region_stmt; - /* We can't free region_stmt below, so let's add it to the stats */ - lsm->stat.memory.count.bytes += tuple_size(entry.stmt); - - /* Abort transaction if format was changed by DDL */ - if (!vy_stmt_is_key(entry.stmt) && - format_id != tuple_format_id(mem->format)) { - diag_set(ClientError, ER_TRANSACTION_CONFLICT); - return -1; - } + struct vy_stmt_counter *count = &lsm->stat.memory.count; if (vy_stmt_type(*region_stmt) != IPROTO_UPSERT) - return vy_mem_insert(mem, entry); + return vy_mem_insert(mem, entry, count); else - return vy_mem_insert_upsert(mem, entry); + return vy_mem_insert_upsert(mem, entry, count); } /** @@ -1019,8 +1017,6 @@ vy_lsm_commit_stmt(struct vy_lsm *lsm, struct vy_mem *mem, { vy_mem_commit_stmt(mem, entry); - lsm->stat.memory.count.rows++; - if (vy_stmt_type(entry.stmt) == IPROTO_UPSERT) vy_lsm_commit_upsert(lsm, mem, entry); @@ -1034,7 +1030,7 @@ void vy_lsm_rollback_stmt(struct vy_lsm *lsm, struct vy_mem *mem, struct vy_entry entry) { - vy_mem_rollback_stmt(mem, entry); + vy_mem_rollback_stmt(mem, entry, &lsm->stat.memory.count); /* Invalidate cache element. */ vy_cache_on_write(&lsm->cache, entry, NULL); diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c index 273c9b7fe40bb59f8cea3802bc14f0c076dfcd15..07bf0d8ced14ef36c00d822e5358127347083510 100644 --- a/src/box/vy_mem.c +++ b/src/box/vy_mem.c @@ -153,7 +153,8 @@ vy_mem_older_lsn(struct vy_mem *mem, struct vy_entry entry) } int -vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry) +vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count) { assert(vy_stmt_type(entry.stmt) == IPROTO_UPSERT); /* Check if the statement can be inserted in the vy_mem. */ @@ -169,9 +170,12 @@ vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry) assert(! vy_mem_tree_iterator_is_invalid(&inserted)); assert(vy_entry_is_equal(entry, *vy_mem_tree_iterator_get_elem(&mem->tree, &inserted))); - if (replaced.stmt == NULL) + if (replaced.stmt == NULL) { mem->count.rows++; + count->rows++; + } mem->count.bytes += size; + count->bytes += size; /* * All iterators begin to see the new statement, and * will be aborted in case of rollback. @@ -213,7 +217,8 @@ vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry) } int -vy_mem_insert(struct vy_mem *mem, struct vy_entry entry) +vy_mem_insert(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count) { assert(vy_stmt_type(entry.stmt) != IPROTO_UPSERT); /* Check if the statement can be inserted in the vy_mem. */ @@ -225,9 +230,12 @@ vy_mem_insert(struct vy_mem *mem, struct vy_entry entry) struct vy_entry replaced = vy_entry_none(); if (vy_mem_tree_insert(&mem->tree, entry, &replaced, NULL)) return -1; - if (replaced.stmt == NULL) + if (replaced.stmt == NULL) { mem->count.rows++; + count->rows++; + } mem->count.bytes += size; + count->bytes += size; /* * All iterators begin to see the new statement, and * will be aborted in case of rollback. @@ -259,16 +267,23 @@ vy_mem_commit_stmt(struct vy_mem *mem, struct vy_entry entry) } void -vy_mem_rollback_stmt(struct vy_mem *mem, struct vy_entry entry) +vy_mem_rollback_stmt(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count) { - /* This is the statement we've inserted before. */ + /* The statement must be from a lsregion. */ assert(!vy_stmt_is_refable(entry.stmt)); - int rc = vy_mem_tree_delete(&mem->tree, entry); - assert(rc == 0); - (void) rc; - /* We can't free memory in case of rollback. */ - mem->count.rows--; - mem->version++; + /* + * The statement isn't present in the memory tree if it was replaced by + * vy_mem_insert(). In this case we must not decrement the row count. + * + * Either way, we don't subtract the statement size because lsregion + * doesn't support freeing memory. + */ + if (vy_mem_tree_delete(&mem->tree, entry) == 0) { + mem->version++; + mem->count.rows--; + count->rows--; + } } /* }}} vy_mem */ diff --git a/src/box/vy_mem.h b/src/box/vy_mem.h index fa1bce3362018bfac7f2f0763dbcdd9e07d8dd31..b1b9c39d5d8195014035cc03697532a8b5225395 100644 --- a/src/box/vy_mem.h +++ b/src/box/vy_mem.h @@ -282,24 +282,28 @@ vy_mem_older_lsn(struct vy_mem *mem, struct vy_entry entry); * Insert a statement into the in-memory level. * @param mem vy_mem. * @param entry Vinyl statement. + * @param count Statement counter to update. * * @retval 0 Success. * @retval -1 Memory error. */ int -vy_mem_insert(struct vy_mem *mem, struct vy_entry entry); +vy_mem_insert(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count); /** * Insert an upsert statement into the mem. * * @param mem Mem to insert to. * @param entry Upsert statement to insert. + * @param count Statement counter to update. * * @retval 0 Success. * @retval -1 Memory error. */ int -vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry); +vy_mem_insert_upsert(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count); /** * Confirm insertion of a statement into the in-memory level. @@ -313,9 +317,11 @@ vy_mem_commit_stmt(struct vy_mem *mem, struct vy_entry entry); * Remove a statement from the in-memory level. * @param mem vy_mem. * @param entry Vinyl statement. + * @param count Statement counter to update. */ void -vy_mem_rollback_stmt(struct vy_mem *mem, struct vy_entry entry); +vy_mem_rollback_stmt(struct vy_mem *mem, struct vy_entry entry, + struct vy_stmt_counter *count); /** * Iterator for in-memory level. diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c index 5c24d9d4d90e70e33768b765bd3da4d511cfbd42..bcadf467410eeffdf4a389e01ca1bd7433bc1a3d 100644 --- a/test/unit/vy_iterators_helper.c +++ b/test/unit/vy_iterators_helper.c @@ -11,6 +11,7 @@ struct vy_stmt_env stmt_env; struct vy_mem_env mem_env; struct vy_cache_env cache_env; struct mempool history_node_pool; +struct vy_stmt_counter dummy_count; void vy_iterator_C_test_init(size_t cache_size) @@ -149,9 +150,9 @@ vy_mem_insert_template(struct vy_mem *mem, const struct vy_stmt_template *templ) tuple_unref(entry.stmt); entry.stmt = region_stmt; if (templ->type == IPROTO_UPSERT) - vy_mem_insert_upsert(mem, entry); + vy_mem_insert_upsert(mem, entry, &dummy_count); else - vy_mem_insert(mem, entry); + vy_mem_insert(mem, entry, &dummy_count); return entry; } diff --git a/test/unit/vy_iterators_helper.h b/test/unit/vy_iterators_helper.h index c6a7f506da64d499c58b08f2c6e8a736abbb67af..a3f7edf430f2bf5bfb9b3108a1af998ad8d1434e 100644 --- a/test/unit/vy_iterators_helper.h +++ b/test/unit/vy_iterators_helper.h @@ -58,6 +58,7 @@ extern struct vy_stmt_env stmt_env; extern struct vy_mem_env mem_env; extern struct vy_cache_env cache_env; extern struct mempool history_node_pool; +extern struct vy_stmt_counter dummy_count; #if defined(__cplusplus) extern "C" { diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c index e91aa458a3b1c744f24777095b7e3b97eac696f1..d3102a69097d98e3a1c19571042bd476f5e5ff92 100644 --- a/test/unit/vy_mem.c +++ b/test/unit/vy_mem.c @@ -42,7 +42,7 @@ test_basic(void) entry = vy_mem_insert_template(mem, &stmts[3]); ok(vy_entry_is_equal(vy_mem_older_lsn(mem, entry), older), "vy_mem_rollback 1"); - vy_mem_rollback_stmt(mem, older); + vy_mem_rollback_stmt(mem, older, &dummy_count); ok(vy_entry_is_equal(vy_mem_older_lsn(mem, entry), olderolder), "vy_mem_rollback 2"); diff --git a/test/vinyl-luatest/gh_10751_duplicate_multikey_stmt_accounting_test.lua b/test/vinyl-luatest/gh_10751_duplicate_multikey_stmt_accounting_test.lua new file mode 100644 index 0000000000000000000000000000000000000000..a7bcd1a8eac9254c05f12d44e48aa10904287cb7 --- /dev/null +++ b/test/vinyl-luatest/gh_10751_duplicate_multikey_stmt_accounting_test.lua @@ -0,0 +1,80 @@ +local server = require('luatest.server') +local t = require('luatest') + +local g = t.group('vinyl.duplicate_multikey_stmt_accounting', + t.helpers.matrix{defer_deletes = {false, true}}) + +g.before_all(function(cg) + cg.server = server:new({ + box_cfg = {vinyl_defer_deletes = cg.params.defer_deletes}, + }) + cg.server:start() +end) + +g.after_all(function(cg) + cg.server:drop() +end) + +g.before_each(function(cg) + cg.server:exec(function() + local s = box.schema.space.create('test', {engine = 'vinyl'}) + s:create_index('primary') + s:create_index('secondary', { + unique = false, + parts = {{'[2][*]', 'unsigned'}}, + }) + end) +end) + +g.after_each(function(cg) + cg.server:exec(function() + if box.space.test ~= nil then + box.space.test:drop() + end + end) +end) + +g.test_commit = function(cg) + cg.server:exec(function() + local s = box.space.test + local i = s.index.secondary + s:replace({1, {1, 1}}) + s:delete({1}) + s:replace({1, {1, 1}}) + box.snapshot() + t.assert_covers(i:stat(), { + memory = {rows = 0, bytes = 0}, + disk = {rows = 1}, + }) + t.assert_equals(i:len(), 1) + end) +end + +g.test_rollback = function(cg) + t.tarantool.skip_if_not_debug() + cg.server:exec(function() + local s = box.space.test + local i = s.index.secondary + s:replace({1, {1, 1}}) + box.begin() + s:delete({1}) + box.error.injection.set('ERRINJ_WAL_WRITE', true) + t.assert_error_covers({ + type = 'ClientError', + code = box.error.WAL_IO, + }, box.commit) + box.error.injection.set('ERRINJ_WAL_WRITE', false) + box.snapshot() + t.assert_covers(i:stat(), { + memory = {rows = 0, bytes = 0}, + disk = {rows = 1}, + }) + t.assert_equals(i:len(), 1) + end) +end + +g.after_test('test_rollback', function(cg) + cg.server:exec(function() + box.error.injection.set('ERRINJ_WAL_WRITE', false) + end) +end) diff --git a/test/vinyl-luatest/select_consistency_test.lua b/test/vinyl-luatest/select_consistency_test.lua index 88184f58da49cca7776342e583d20b94f6e95e8a..b002b52045d927f85c8672a689a06af7a4d68f39 100644 --- a/test/vinyl-luatest/select_consistency_test.lua +++ b/test/vinyl-luatest/select_consistency_test.lua @@ -232,8 +232,17 @@ g.test_select_consistency = function(cg) t.assert_equals(box.stat.vinyl().scheduler.compaction_queue, 0) t.assert_equals(box.stat.vinyl().scheduler.tasks_inprogress, 0) end) - t.assert_equals(s.index.i1:len(), s.index.i1:count()) - t.assert_equals(s.index.i2:len(), s.index.i3:len()) - t.assert_equals(s.index.i4:len(), s.index.i5:len()) + local c1 = s.index.i1:count() + local c2 = s.index.i2:count() + local c3 = s.index.i3:count() + local c4 = s.index.i4:count() + local c5 = s.index.i5:count() + t.assert_equals(s.index.i1:len(), c1) + t.assert_equals(s.index.i2:len(), c2) + t.assert_equals(s.index.i3:len(), c3) + t.assert_equals(s.index.i4:len(), c4) + t.assert_equals(s.index.i5:len(), c5) + t.assert_equals(c2, c3) + t.assert_equals(c4, c5) end) end