[PATCH v2 3/3] vinyl: don't track tuples that are already tracked in secondary index

Vladimir Davydov vdavydov.dev at gmail.com
Wed Mar 7 15:06:33 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                 | 15 ++++++++++--
 src/box/vy_point_lookup.c       | 12 ---------
 src/box/vy_point_lookup.h       |  9 +++++++
 test/vinyl/tx_gap_lock.result   | 54 +++++++++++++++++++++++++++++++++++++++++
 test/vinyl/tx_gap_lock.test.lua | 19 +++++++++++++++
 5 files changed, 95 insertions(+), 14 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index d5b45f6f..3467281a 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1347,8 +1347,11 @@ 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)
+	if (tuple_field_count(key) >= index->cmp_def->part_count) {
+		if (tx != NULL && vy_tx_track_point(tx, index, key) != 0)
+			return -1;
 		return vy_point_lookup(index, tx, rv, key, result);
+	}
 
 	struct vy_read_iterator itr;
 	vy_read_iterator_open(&itr, index, tx, ITER_EQ, key, rv);
@@ -1696,6 +1699,10 @@ vy_index_full_by_key(struct vy_index *index, struct vy_tx *tx,
 		*result = found;
 		return 0;
 	}
+	/*
+	 * No need in vy_tx_track() as the tuple is already
+	 * tracked in the secondary index.
+	 */
 	rc = vy_point_lookup(index->pk, tx, rv, found, result);
 	tuple_unref(found);
 	return rc;
@@ -3926,7 +3933,11 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret)
 			fiber_sleep(0.01);
 	}
 #endif
-	/* Get the full tuple from the primary index. */
+	/*
+	 * Get the full tuple from the primary index.
+	 * Note, there's no need in vy_tx_track() as the
+	 * tuple is already tracked in the secondary index.
+	 */
 	if (vy_point_lookup(it->index->pk, it->tx, vy_tx_read_view(it->tx),
 			    tuple, &tuple) != 0)
 		goto fail;
diff --git a/src/box/vy_point_lookup.c b/src/box/vy_point_lookup.c
index 4076d231..ab0bc6b8 100644
--- a/src/box/vy_point_lookup.c
+++ b/src/box/vy_point_lookup.c
@@ -402,18 +402,6 @@ vy_point_lookup(struct vy_index *index, struct vy_tx *tx,
 	index->stat.lookup++;
 	/* History list */
 	struct rlist history;
-	/*
-	 * Notify the TX manager that we are about to read the key
-	 * so that if a new statement with the same key arrives
-	 * while we are reading a run file, we will be sent to a
-	 * read view and hence will not try to add a stale value
-	 * to the cache.
-	 */
-	if (tx != NULL) {
-		rc = vy_tx_track_point(tx, index, key);
-		if (rc != 0)
-			goto done;
-	}
 restart:
 	rlist_create(&history);
 
diff --git a/src/box/vy_point_lookup.h b/src/box/vy_point_lookup.h
index 632b075b..a4dbe77e 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,6 +61,13 @@ 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.
+ *
+ * The caller must guarantee that if the tuple looked up by this
+ * function is modified, the transaction will be sent to read view.
+ * This is needed to avoid inserting a stale value into the cache.
+ * In other words, vy_tx_track() must be called for the search key
+ * before calling this function unless this is a primary index and
+ * the tuple is already tracked in a secondary index.
  */
 int
 vy_point_lookup(struct vy_index *index, struct vy_tx *tx,
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