From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated Date: Sat, 25 May 2019 00:53:41 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: To: tarantool-patches@freelists.org List-ID: 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