From: Konstantin Osipov <kostja@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH 1/1] vinyl: fix crash in vinyl_iterator_secondary_next Date: Sat, 12 May 2018 02:52:08 +0300 [thread overview] Message-ID: <20180511235208.GA901@atlas> (raw) In-Reply-To: <dff067540ee7081cc4e488f283b7475f72b0c937.1526080999.git.v.shpilevoy@tarantool.org> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/05/12 02:27]: > Vinyl_iterator_secondary_next is a method to iterate over a > secondary index. It gets a tuple from the secondary index and > looks up its full version in the primary index. > > But during lookup the tuple can be deleted from the former, and > the iterator must take it in the hopper. > > Closes #3393 First, is 1.9 affected? My guess is it isn't and this is a regression from Vova's refactoring for online after. The key to the problem seems to be in the exact timing when the key is added to the transaction conflict manager by vy_tx_track(). But in this case your fix seems to be rather crude, the problem should be solved by harmonizing the timing of tracking and lookup in the primary index. I got in touch with Vova and he confirmed that the read should be tracked before a yield. Let's wait for his root cause analysis, he promised to take a look at the test case/patch. > src/box/vinyl.c | 8 ++++- > test/vinyl/errinj.result | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > test/vinyl/errinj.test.lua | 26 +++++++++++++++++ > 3 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > index ff4c283..460b9b5 100644 > --- a/src/box/vinyl.c > +++ b/src/box/vinyl.c > @@ -3853,7 +3853,7 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret) > goto fail; > } > > - > +next_tuple: > if (vy_read_iterator_next(&it->iterator, &tuple) != 0) > goto fail; > > @@ -3879,6 +3879,12 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret) > if (vy_point_lookup(it->lsm->pk, it->tx, vy_tx_read_view(it->tx), > tuple, &tuple) != 0) > goto fail; > + /* > + * The tuple can be removed by the same transaction during > + * primary index lookup. > + */ > + if (tuple == NULL) > + goto next_tuple; > *ret = tuple_bless(tuple); > tuple_unref(tuple); > if (*ret != NULL) > diff --git a/test/vinyl/errinj.result b/test/vinyl/errinj.result > index fd21f7b..b63d01c 100644 > --- a/test/vinyl/errinj.result > +++ b/test/vinyl/errinj.result > @@ -1395,3 +1395,76 @@ s:count() -- 200 > s:drop() > --- > ... > +-- > +-- gh-3393: vinyl secondary index iterator must take in the hopper > +-- that a tuple can disappear from the primary index after the > +-- secondary index lookup. > +-- > +s = box.schema.create_space('test', {engine = 'vinyl'}) > +--- > +... > +pk = s:create_index('pk') > +--- > +... > +sk = s:create_index('sk', {parts = {{2, 'unsigned'}, {1, 'unsigned'}}}) > +--- > +... > +s:replace{1, 2} > +--- > +- [1, 2] > +... > +s:replace{3, 4} > +--- > +- [3, 4] > +... > +box.snapshot() > +--- > +- ok > +... > +ret = nil > +--- > +... > +function iterate(i) ret = i.next() end > +--- > +... > +box.begin() > +--- > +... > +s:replace{5, 6} -- Start a transaction in the engine. > +--- > +- [5, 6] > +... > +iter = create_iterator(sk, {4}, {iterator = 'GE'}) > +--- > +... > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", true) > +--- > +- ok > +... > +f = fiber.create(iterate, iter) > +--- > +... > +s:delete{3} > +--- > +... > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", false) > +--- > +- ok > +... > +while not ret do fiber.sleep(0.1) end > +--- > +... > +ret > +--- > +- [5, 6] > +... > +iter.iterate_over() > +--- > +- [] > +... > +box.commit() > +--- > +... > +s:drop() > +--- > +... > diff --git a/test/vinyl/errinj.test.lua b/test/vinyl/errinj.test.lua > index 64d04c6..c551de1 100644 > --- a/test/vinyl/errinj.test.lua > +++ b/test/vinyl/errinj.test.lua > @@ -548,3 +548,29 @@ ch:get() > > s:count() -- 200 > s:drop() > + > +-- > +-- gh-3393: vinyl secondary index iterator must take in the hopper > +-- that a tuple can disappear from the primary index after the > +-- secondary index lookup. > +-- > +s = box.schema.create_space('test', {engine = 'vinyl'}) > +pk = s:create_index('pk') > +sk = s:create_index('sk', {parts = {{2, 'unsigned'}, {1, 'unsigned'}}}) > +s:replace{1, 2} > +s:replace{3, 4} > +box.snapshot() > +ret = nil > +function iterate(i) ret = i.next() end > +box.begin() > +s:replace{5, 6} -- Start a transaction in the engine. > +iter = create_iterator(sk, {4}, {iterator = 'GE'}) > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", true) > +f = fiber.create(iterate, iter) > +s:delete{3} > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", false) > +while not ret do fiber.sleep(0.1) end > +ret > +iter.iterate_over() > +box.commit() > +s:drop() > > -- > To stop receiving notification emails like this one, please contact > the administrator of this repository. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov > Vinyl_iterator_secondary_next is a method to iterate over a > secondary index. It gets a tuple from the secondary index and > looks up its full version in the primary index. > > But during lookup the tuple can be deleted from the former, and > the iterator must take it in the hopper. > > Closes #3393 > --- > Branch: https://github.com/tarantool/tarantool/tree/gh-3393-vinyl-iterator-crash > Issue: https://github.com/tarantool/tarantool/issues/3393 > > src/box/vinyl.c | 8 ++++- > test/vinyl/errinj.result | 73 ++++++++++++++++++++++++++++++++++++++++++++++ > test/vinyl/errinj.test.lua | 26 +++++++++++++++++ > 3 files changed, 106 insertions(+), 1 deletion(-) > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > index ff4c28314..460b9b5e7 100644 > --- a/src/box/vinyl.c > +++ b/src/box/vinyl.c > @@ -3853,7 +3853,7 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret) > goto fail; > } > > - > +next_tuple: > if (vy_read_iterator_next(&it->iterator, &tuple) != 0) > goto fail; > > @@ -3879,6 +3879,12 @@ vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret) > if (vy_point_lookup(it->lsm->pk, it->tx, vy_tx_read_view(it->tx), > tuple, &tuple) != 0) > goto fail; > + /* > + * The tuple can be removed by the same transaction during > + * primary index lookup. > + */ > + if (tuple == NULL) > + goto next_tuple; > *ret = tuple_bless(tuple); > tuple_unref(tuple); > if (*ret != NULL) > diff --git a/test/vinyl/errinj.result b/test/vinyl/errinj.result > index fd21f7bb4..b63d01c18 100644 > --- a/test/vinyl/errinj.result > +++ b/test/vinyl/errinj.result > @@ -1395,3 +1395,76 @@ s:count() -- 200 > s:drop() > --- > ... > +-- > +-- gh-3393: vinyl secondary index iterator must take in the hopper > +-- that a tuple can disappear from the primary index after the > +-- secondary index lookup. > +-- > +s = box.schema.create_space('test', {engine = 'vinyl'}) > +--- > +... > +pk = s:create_index('pk') > +--- > +... > +sk = s:create_index('sk', {parts = {{2, 'unsigned'}, {1, 'unsigned'}}}) > +--- > +... > +s:replace{1, 2} > +--- > +- [1, 2] > +... > +s:replace{3, 4} > +--- > +- [3, 4] > +... > +box.snapshot() > +--- > +- ok > +... > +ret = nil > +--- > +... > +function iterate(i) ret = i.next() end > +--- > +... > +box.begin() > +--- > +... > +s:replace{5, 6} -- Start a transaction in the engine. > +--- > +- [5, 6] > +... > +iter = create_iterator(sk, {4}, {iterator = 'GE'}) > +--- > +... > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", true) > +--- > +- ok > +... > +f = fiber.create(iterate, iter) > +--- > +... > +s:delete{3} > +--- > +... > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", false) > +--- > +- ok > +... > +while not ret do fiber.sleep(0.1) end > +--- > +... > +ret > +--- > +- [5, 6] > +... > +iter.iterate_over() > +--- > +- [] > +... > +box.commit() > +--- > +... > +s:drop() > +--- > +... > diff --git a/test/vinyl/errinj.test.lua b/test/vinyl/errinj.test.lua > index 64d04c62d..c551de1b3 100644 > --- a/test/vinyl/errinj.test.lua > +++ b/test/vinyl/errinj.test.lua > @@ -548,3 +548,29 @@ ch:get() > > s:count() -- 200 > s:drop() > + > +-- > +-- gh-3393: vinyl secondary index iterator must take in the hopper > +-- that a tuple can disappear from the primary index after the > +-- secondary index lookup. > +-- > +s = box.schema.create_space('test', {engine = 'vinyl'}) > +pk = s:create_index('pk') > +sk = s:create_index('sk', {parts = {{2, 'unsigned'}, {1, 'unsigned'}}}) > +s:replace{1, 2} > +s:replace{3, 4} > +box.snapshot() > +ret = nil > +function iterate(i) ret = i.next() end > +box.begin() > +s:replace{5, 6} -- Start a transaction in the engine. > +iter = create_iterator(sk, {4}, {iterator = 'GE'}) > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", true) > +f = fiber.create(iterate, iter) > +s:delete{3} > +errinj.set("ERRINJ_VY_DELAY_PK_LOOKUP", false) > +while not ret do fiber.sleep(0.1) end > +ret > +iter.iterate_over() > +box.commit() > +s:drop() > -- > 2.15.1 (Apple Git-101) -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov
next prev parent reply other threads:[~2018-05-11 23:52 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-11 23:24 [tarantool-patches] " Vladislav Shpilevoy 2018-05-11 23:52 ` Konstantin Osipov [this message] 2018-05-12 0:30 ` Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180511235208.GA901@atlas \ --to=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=v.shpilevoy@tarantool.org \ --subject='[tarantool-patches] Re: [PATCH 1/1] vinyl: fix crash in vinyl_iterator_secondary_next' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox