From 26a3c8cfafb072977d8c71d1f1f16a4f5d81a9c9 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Mon, 11 Nov 2024 18:14:24 +0300
Subject: [PATCH] vinyl: fix duplicate multikey stmt accounting with deferred
deletes
`vy_mem_insert()` and `vy_mem_insert_upsert()` increment the row count
statistic of `vy_mem` only if no statement is replaced, which is
correct, while `vy_lsm_commit()` increments the row count of `vy_lsm`
unconditionally. As a result, `vy_lsm` may report a non-zero statement
count (via `index.stat()` or `index.len()`) after a dump. This may
happen only with a non-unique multikey index, when the statement has
duplicates in the indexed array, and only if the `deferred_deletes`
option is enabled, because otherwise we drop duplicates when we form
the transaction write set, see `vy_tx_set()`. With `deferred_deletes`,
we may create a `txv` for each multikey entry at the time when we
prepare to commit the transaction, see `vy_tx_handle_deferred_delete()`.
Another problem is that `vy_mem_rollback_stmt()` always decrements
the row count, even if it didn't find the rolled back statement in
the tree. As a result, if the transaction with duplicate multikey
entries is rolled back on WAL error, we'll decrement the row count
of `vy_mem` more times than necessary.
To fix this issue, let's make the `vy_mem` methods update the in-memory
statistic of `vy_lsm`. This way they should always stay in-sync. Also,
we make `vy_mem_rollback_stmt()` skip updating the statistics in case
the rolled back statement isn't present in the tree.
This issue results in `vinyl-luatest/select_consistency_test.lua`
flakiness when checking `index.len()` after compaction. Let's make
the test more thorough and also check that `index.len()` equals
`index.count()`.
Closes #10751
Part of #10752
NO_DOC=bug fix
(cherry picked from commit e8810c555d4e6ba56e6c798e04216aa11efb5304)
---
...-10751-fix-vinyl-memory-stmt-accounting.md | 5 ++
src/box/vinyl.c | 4 +-
src/box/vy_lsm.c | 24 +++---
src/box/vy_mem.c | 39 ++++++---
src/box/vy_mem.h | 12 ++-
test/unit/vy_iterators_helper.c | 5 +-
test/unit/vy_iterators_helper.h | 1 +
test/unit/vy_mem.c | 2 +-
...uplicate_multikey_stmt_accounting_test.lua | 80 +++++++++++++++++++
.../vinyl-luatest/select_consistency_test.lua | 15 +++-
10 files changed, 149 insertions(+), 38 deletions(-)
create mode 100644 changelogs/unreleased/gh-10751-fix-vinyl-memory-stmt-accounting.md
create mode 100644 test/vinyl-luatest/gh_10751_duplicate_multikey_stmt_accounting_test.lua
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 0000000000..830bb55940
--- /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 ebe59d4c0a..018f2a409b 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 24aa50b52d..f4468aed62 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 273c9b7fe4..07bf0d8ced 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 fa1bce3362..b1b9c39d5d 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 5c24d9d4d9..bcadf46741 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 c6a7f506da..a3f7edf430 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 e91aa458a3..d3102a6909 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 0000000000..a7bcd1a8ea
--- /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 88184f58da..b002b52045 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
--
GitLab