From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 704EB301EC for ; Sat, 25 May 2019 02:13:42 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id mpOSDo6soltj for ; Sat, 25 May 2019 02:13:42 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 090F4301EA for ; Sat, 25 May 2019 02:13:41 -0400 (EDT) Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1hUPw4-0003Hr-4H for tarantool-patches@freelists.org; Sat, 25 May 2019 09:13:40 +0300 Date: Sat, 25 May 2019 09:13:39 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 2/3] vinyl: don't produce deferred DELETE on commit if key isn't updated Message-ID: <20190525061339.GC14501@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org * Vladimir Davydov [19/05/25 06:41]: this one is basically lgtm > 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 > -- Konstantin Osipov, Moscow, Russia