[PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated

Vladimir Davydov vdavydov.dev at gmail.com
Sat May 25 00:53:41 MSK 2019


Consider the following example:

  s = box.schema.space.create('test', {engine = 'vinyl'})
  s:create_index('primary')
  s:create_index('secondary', {parts = {2, 'unsigned'}})

  s:insert{1, 1, 1}
  s:replace{1, 1, 2}

When REPLACE{1,1} is committed to the secondary index, the overwritten
tuple, i.e. INSERT{1,1}, is found in the primary index memory, and so
deferred DELETE{1,1} is generated right away and committed along with
REPLACE{1,1}. However, there's no need to commit anything to the
secondary index in this case, because its key isn't updated. Apart from
eating memory and loading disk, this also breaks index stats, as vy_tx
implementation doesn't expect two statements committed for the same key
in a single transaction.

Fix this by checking if there's a statement in the log for the deleted
key and if there's skipping them both as we do in the regular case, see
the comment in vy_tx_set.

Closes #3693
---
 src/box/vy_tx.c                     | 18 +++++++++
 test/vinyl/deferred_delete.result   | 78 +++++++++++++++++++++++++++++++++++++
 test/vinyl/deferred_delete.test.lua | 26 +++++++++++++
 test/vinyl/quota.result             |  2 +-
 test/vinyl/stat.result              |  6 +--
 test/vinyl/stat.test.lua            |  2 +-
 test/vinyl/write_iterator.result    |  2 +-
 test/vinyl/write_iterator.test.lua  |  2 +-
 8 files changed, 129 insertions(+), 7 deletions(-)

diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index aedd0610..119d975b 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -629,6 +629,23 @@ vy_tx_handle_deferred_delete(struct vy_tx *tx, struct txv *v)
 		struct vy_lsm *lsm = vy_lsm(space->index[i]);
 		struct vy_entry entry;
 		vy_stmt_foreach_entry(entry, delete_stmt, lsm->cmp_def) {
+			struct txv *other = write_set_search_key(&tx->write_set,
+								 lsm, entry);
+			if (other != NULL && !other->is_overwritten) {
+				/*
+				 * The write set contains a statement
+				 * for the key to be deleted. This can
+				 * only occur if it's a REPLACE that
+				 * happens not to update the secondary
+				 * index key parts. It's safe to skip it,
+				 * see vy_tx_set_entry().
+				 */
+				assert(vy_stmt_type(stmt) == IPROTO_REPLACE);
+				assert(vy_stmt_type(other->entry.stmt) ==
+								IPROTO_REPLACE);
+				other->is_nop = true;
+				continue;
+			}
 			struct txv *delete_txv = txv_new(tx, lsm, entry);
 			if (delete_txv == NULL) {
 				rc = -1;
@@ -717,6 +734,7 @@ vy_tx_prepare(struct vy_tx *tx)
 			 * to the primary index LSM tree. So we must
 			 * skip it for secondary indexes as well.
 			 */
+			v->is_overwritten = true;
 			continue;
 		}
 
diff --git a/test/vinyl/deferred_delete.result b/test/vinyl/deferred_delete.result
index 422f2fe0..3f187a5b 100644
--- a/test/vinyl/deferred_delete.result
+++ b/test/vinyl/deferred_delete.result
@@ -726,6 +726,84 @@ sk:select() -- ditto
 s:drop()
 ---
 ...
+--
+-- gh-3693 If a REPLACE doesn't update secondary index key parts,
+-- we must not generate a deferred DELETE for it on commit.
+-- Moreover, we must skip it as it has no effect.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
+s:insert{1, 1, 1}
+---
+- [1, 1, 1]
+...
+s:insert{2, 2, 2}
+---
+- [2, 2, 2]
+...
+s:replace{2, 2, 3}
+---
+- [2, 2, 3]
+...
+pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
+---
+- 3
+...
+sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
+---
+- 2
+...
+box.begin()
+---
+...
+s:replace{1, 1, 2}
+---
+- [1, 1, 2]
+...
+s:delete{1}
+---
+...
+box.commit()
+---
+...
+pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
+---
+- 4
+...
+sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
+---
+- 3
+...
+box.snapshot()
+---
+- ok
+...
+pk:stat().rows -- 1: INSERT{2,2,3}
+---
+- 1
+...
+sk:stat().rows -- 1: INSERT{2,2}
+---
+- 1
+...
+pk:select()
+---
+- - [2, 2, 3]
+...
+sk:select()
+---
+- - [2, 2, 3]
+...
+s:drop()
+---
+...
 box.cfg{vinyl_cache = vinyl_cache}
 ---
 ...
diff --git a/test/vinyl/deferred_delete.test.lua b/test/vinyl/deferred_delete.test.lua
index 6dfcbde5..689c8f93 100644
--- a/test/vinyl/deferred_delete.test.lua
+++ b/test/vinyl/deferred_delete.test.lua
@@ -265,6 +265,32 @@ box.snapshot()
 sk:select() -- ditto
 s:drop()
 
+--
+-- gh-3693 If a REPLACE doesn't update secondary index key parts,
+-- we must not generate a deferred DELETE for it on commit.
+-- Moreover, we must skip it as it has no effect.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+pk = s:create_index('pk')
+sk = s:create_index('sk', {parts = {2, 'unsigned'}})
+s:insert{1, 1, 1}
+s:insert{2, 2, 2}
+s:replace{2, 2, 3}
+pk:stat().rows -- 3: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3}
+sk:stat().rows -- 2: INSERT{1,1} + INSERT{2,2}
+box.begin()
+s:replace{1, 1, 2}
+s:delete{1}
+box.commit()
+pk:stat().rows -- 4: INSERT{1,1,1} + INSERT{2,2,2} + REPLACE{2,2,3} + DELETE{1}
+sk:stat().rows -- 3: INSERT{1,1} + INSERT{2,2} + DELETE{1,1}
+box.snapshot()
+pk:stat().rows -- 1: INSERT{2,2,3}
+sk:stat().rows -- 1: INSERT{2,2}
+pk:select()
+sk:select()
+s:drop()
+
 box.cfg{vinyl_cache = vinyl_cache}
 
 --
diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
index 1a0842f9..d1b28ee5 100644
--- a/test/vinyl/quota.result
+++ b/test/vinyl/quota.result
@@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 * 5)}
 ...
 box.stat.vinyl().memory.level0
 ---
-- 5341267
+- 5292076
 ...
 space:drop()
 ---
diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
index aa47a7be..e2991d53 100644
--- a/test/vinyl/stat.result
+++ b/test/vinyl/stat.result
@@ -1158,7 +1158,7 @@ s = box.schema.space.create('test', {engine = 'vinyl'})
 i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
 ---
 ...
-i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
+i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
 ---
 ...
 for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
@@ -1191,7 +1191,7 @@ box.snapshot()
 stat_diff(gstat(), st, 'scheduler')
 ---
 - dump_input: 10420
-  dump_output: 10371
+  dump_output: 10411
   tasks_completed: 2
   dump_count: 1
 ...
@@ -1206,7 +1206,7 @@ while i1:stat().disk.compaction.count == 0 do fiber.sleep(0.01) end
 ...
 stat_diff(gstat(), st, 'scheduler')
 ---
-- compaction_input: 112188
+- compaction_input: 112228
   tasks_completed: 1
   compaction_output: 101984
 ...
diff --git a/test/vinyl/stat.test.lua b/test/vinyl/stat.test.lua
index 4a360f33..4c751aad 100644
--- a/test/vinyl/stat.test.lua
+++ b/test/vinyl/stat.test.lua
@@ -333,7 +333,7 @@ s:drop()
 -- sched stats
 s = box.schema.space.create('test', {engine = 'vinyl'})
 i1 = s:create_index('i1', {parts = {1, 'unsigned'}})
-i2 = s:create_index('i2', {parts = {2, 'unsigned'}})
+i2 = s:create_index('i2', {parts = {2, 'unsigned'}, unique = false})
 
 for i = 1, 100 do s:replace{i, i, string.rep('x', 1000)} end
 st = gstat()
diff --git a/test/vinyl/write_iterator.result b/test/vinyl/write_iterator.result
index 5c5200b8..f0244113 100644
--- a/test/vinyl/write_iterator.result
+++ b/test/vinyl/write_iterator.result
@@ -931,7 +931,7 @@ box.snapshot()
 - ok
 ...
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 ---
 ...
 box.snapshot()
diff --git a/test/vinyl/write_iterator.test.lua b/test/vinyl/write_iterator.test.lua
index fa44961a..0de21f21 100644
--- a/test/vinyl/write_iterator.test.lua
+++ b/test/vinyl/write_iterator.test.lua
@@ -389,7 +389,7 @@ _ = s:insert{7, 7}
 _ = s:insert{8, 8}
 box.snapshot()
 -- Some padding to trigger minor compaction.
-for i = 1001, 1000 + PAD2 do s:replace{i, i} end
+for i = 1001, 1000 + PAD2 do s:delete{i} end
 box.snapshot()
 -- Generate DELETE+INSERT statements and write them to disk.
 s:delete{1} s:insert{1, 100}
-- 
2.11.0




More information about the Tarantool-patches mailing list