[PATCH 3/3] vinyl: don't track tuples that are already tracked in secondary index
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Mar 7 12:05:11 MSK 2018
When scanning a secondary index, we actually track each tuple in the
transaction manager twice - as a part of the interval read from the
secondary index and as a point in the primary index when retrieving
the full tuple. This bloats the read set - instead of storing just one
interval for a range request, we also store each tuple returned by it,
which may count to thousands. There's no point in this extra tracking,
because whenever we change a tuple in the primary index, we also update
it in all secondary indexes. So let's remove it to save us some memory
and cpu cycles.
This is an alternative fix for #2534
It should also mitigate #3197
---
src/box/vinyl.c | 8 +++---
src/box/vy_point_lookup.c | 6 ++---
src/box/vy_point_lookup.h | 10 ++++++--
test/unit/vy_point_lookup.c | 2 +-
test/vinyl/tx_gap_lock.result | 54 +++++++++++++++++++++++++++++++++++++++++
test/vinyl/tx_gap_lock.test.lua | 19 +++++++++++++++
6 files changed, 89 insertions(+), 10 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 32781ad2..2af4bc9e 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1348,7 +1348,7 @@ vy_index_get(struct vy_index *index, struct vy_tx *tx,
assert(tx == NULL || tx->state == VINYL_TX_READY);
if (tuple_field_count(key) >= index->cmp_def->part_count)
- return vy_point_lookup(index, tx, rv, key, result);
+ return vy_point_lookup(index, tx, rv, key, false, result);
struct vy_read_iterator itr;
vy_read_iterator_open(&itr, index, tx, ITER_EQ, key, rv);
@@ -1690,7 +1690,7 @@ vy_index_full_by_key(struct vy_index *index, struct vy_tx *tx,
*result = found;
return 0;
}
- rc = vy_point_lookup(index->pk, tx, rv, found, result);
+ rc = vy_point_lookup(index->pk, tx, rv, found, true, result);
tuple_unref(found);
return rc;
}
@@ -3575,7 +3575,7 @@ vy_squash_process(struct vy_squash *squash)
*/
struct tuple *result;
if (vy_point_lookup(index, NULL, &env->xm->p_committed_read_view,
- squash->stmt, &result) != 0)
+ squash->stmt, false, &result) != 0)
return -1;
if (result == NULL)
return 0;
@@ -3889,7 +3889,7 @@ vinyl_iterator_next(struct iterator *base, struct tuple **ret)
/* Get the full tuple from the primary index. */
if (vy_point_lookup(it->index->pk, it->tx,
vy_tx_read_view(it->tx),
- tuple, &tuple) != 0)
+ tuple, true, &tuple) != 0)
goto fail;
} else {
tuple_ref(tuple);
diff --git a/src/box/vy_point_lookup.c b/src/box/vy_point_lookup.c
index 4076d231..beb84d16 100644
--- a/src/box/vy_point_lookup.c
+++ b/src/box/vy_point_lookup.c
@@ -389,8 +389,8 @@ vy_point_lookup_apply_history(struct vy_index *index,
int
vy_point_lookup(struct vy_index *index, struct vy_tx *tx,
- const struct vy_read_view **rv,
- struct tuple *key, struct tuple **ret)
+ const struct vy_read_view **rv, struct tuple *key,
+ bool key_is_tracked_in_tx, struct tuple **ret)
{
assert(tuple_field_count(key) >= index->cmp_def->part_count);
@@ -409,7 +409,7 @@ vy_point_lookup(struct vy_index *index, struct vy_tx *tx,
* read view and hence will not try to add a stale value
* to the cache.
*/
- if (tx != NULL) {
+ if (tx != NULL && !key_is_tracked_in_tx) {
rc = vy_tx_track_point(tx, index, key);
if (rc != 0)
goto done;
diff --git a/src/box/vy_point_lookup.h b/src/box/vy_point_lookup.h
index 632b075b..113f5f17 100644
--- a/src/box/vy_point_lookup.h
+++ b/src/box/vy_point_lookup.h
@@ -45,6 +45,8 @@
* and, if the result is the latest version of the key, adds it to cache.
*/
+#include <stdbool.h>
+
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
@@ -59,11 +61,15 @@ struct tuple;
* parts in case of a secondary index), lookup the corresponding
* tuple in the index. The tuple is returned in @ret with its
* reference counter elevated.
+ *
+ * If @key_is_tracked_in_tx is set, the tuple corresponding to
+ * the search key is already tracked in a secondary index and
+ * hence we don't need to track it again.
*/
int
vy_point_lookup(struct vy_index *index, struct vy_tx *tx,
- const struct vy_read_view **rv,
- struct tuple *key, struct tuple **ret);
+ const struct vy_read_view **rv, struct tuple *key,
+ bool key_is_tracked_in_tx, struct tuple **ret);
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index 52f4427e..2385f011 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -280,7 +280,7 @@ test_basic()
pk->mem_format_with_colmask,
&tmpl_key);
struct tuple *res;
- rc = vy_point_lookup(pk, NULL, &prv, key, &res);
+ rc = vy_point_lookup(pk, NULL, &prv, key, false, &res);
tuple_unref(key);
if (rc != 0) {
has_errors = true;
diff --git a/test/vinyl/tx_gap_lock.result b/test/vinyl/tx_gap_lock.result
index 2a5087bf..a1275131 100644
--- a/test/vinyl/tx_gap_lock.result
+++ b/test/vinyl/tx_gap_lock.result
@@ -1193,6 +1193,60 @@ c4:commit()
s:drop()
---
...
+----------------------------------------------------------------
+-- gh-2534: Iterator over a secondary index doesn't double track
+-- results in the primary index.
+----------------------------------------------------------------
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk', {parts = {1, 'unsigned'}})
+---
+...
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+for i = 1, 100 do s:insert{i, i} end
+---
+...
+box.begin()
+---
+...
+gap_lock_count() -- 0
+---
+- 0
+...
+_ = s.index.sk:select({}, {limit = 50})
+---
+...
+gap_lock_count() -- 1
+---
+- 1
+...
+for i = 1, 100 do s.index.sk:get(i) end
+---
+...
+gap_lock_count() -- 51
+---
+- 51
+...
+_ = s.index.sk:select()
+---
+...
+gap_lock_count() -- 1
+---
+- 1
+...
+box.commit()
+---
+...
+gap_lock_count() -- 0
+---
+- 0
+...
+s:drop()
+---
+...
gap_lock_count = nil
---
...
diff --git a/test/vinyl/tx_gap_lock.test.lua b/test/vinyl/tx_gap_lock.test.lua
index 2343a719..d968349b 100644
--- a/test/vinyl/tx_gap_lock.test.lua
+++ b/test/vinyl/tx_gap_lock.test.lua
@@ -379,6 +379,25 @@ c3:commit()
c4:commit()
s:drop()
+----------------------------------------------------------------
+-- gh-2534: Iterator over a secondary index doesn't double track
+-- results in the primary index.
+----------------------------------------------------------------
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {parts = {1, 'unsigned'}})
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+for i = 1, 100 do s:insert{i, i} end
+box.begin()
+gap_lock_count() -- 0
+_ = s.index.sk:select({}, {limit = 50})
+gap_lock_count() -- 1
+for i = 1, 100 do s.index.sk:get(i) end
+gap_lock_count() -- 51
+_ = s.index.sk:select()
+gap_lock_count() -- 1
+box.commit()
+gap_lock_count() -- 0
+s:drop()
gap_lock_count = nil
----------------------------------------------------------------
--
2.11.0
More information about the Tarantool-patches
mailing list