From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 015FA469719 for ; Tue, 3 Mar 2020 17:17:56 +0300 (MSK) Date: Tue, 3 Mar 2020 17:17:56 +0300 From: Sergey Ostanevich Message-ID: <20200303141756.GC188@tarantool.org> References: <20200210074712.86876-1-arkholga@tarantool.org> <20200210111826.GD1110@tarantool.org> <2cbb1677-039b-9f91-cde7-9c06821653eb@tarantool.org> <20200211124245.GB5168@tarantool.org> <20200218124612.GA67746@tarantool.org> <714780e3-1383-62da-94f9-0e90542d47aa@tarantool.org> <20200219162843.GA68447@tarantool.org> <78db8579-e246-385b-6541-5cc70395be97@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <78db8579-e246-385b-6541-5cc70395be97@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org Olga, Thanks for explanation! LGTM. Sergos On 03 мар 14:50, Olga Arkhangelskaia wrote: > Hi Sergey! I need time to check everything! > > I can not use error injection that exists in memtx_index_extent_reserve(), > > because it is used  in memtx_space_replace_all_keys ant it is called prior > memtx_rtree_index_reserve() . > > If we use memtx_index_extent_reserve(), injection we will not reach the > place where error with rtree occures. > > So to make sure that we fixed the problem we need > memtx_rtree_index_reserve() to fail. > > > 19.02.2020 19:28, Sergey Ostanevich пишет: > > Hi! > > > > I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly > > branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly > > > > I can see memtx_index_extent_reserve() has an error injection already. > > Is it not sufficient, so you still need a new one in > > memtx_rtree_index_reserve() ? > > > Otherwise looks good. > > > > Regards, > > Sergos > > > > > > On 18 фев 21:20, Olga Arkhangelskaia wrote: > > > Hi! Thanks for the review! But I am a bit confused. > > > > > > 18.02.2020 15:46, Sergey Ostanevich пишет: > > > > Hi! > > > > > > > > Thanks for the patch! > > > > > > > > I wonder why do you use additional by-ref argument to pass an integer > > > > result instead of redefining the interface into: > > > > > > > > int > > > > rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj) > > > > > > > > instead, so that all uses of a form: > > > > > > > > int err = 0; > > > > ... > > > > rtree_insert(&index->tree, &rect, new_tuple, &err); > > > > > > There is no way rtree returns an error. Library itself can not fail.  And > > > the best way is to reserve pages before malloc itself in the library. > > > > > > We have discussed in the very beginning with Konstantin Osipov and later > > > with Aleksandr Lyapunov. > > > > > > https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html > > > > > > So we reserve memory instead of returning an error. > > > > > > > if (err == 1) { > > > > diag_set(OutOfMemory, sizeof(struct rtree), > > > > "rtree", "rtree_page"); > > > > return -1; > > > > } > > > > > > > > into a much simpler: > > > > > > > > if (rtree_insert(&index->tree, &rect, new_tuple, &err)) { > > > > diag_set(OutOfMemory, sizeof(struct rtree), > > > > "rtree", "rtree_page"); > > > > return -1; > > > > } > > > > > > > > Sergos > > > > > > > > On 11 Feb 15:42, Nikita Pettik wrote: > > > > > On 11 Feb 13:51, Olga Arkhangelskaia wrote: > > > > > > Hi Nikita! Thanks for the review. > > > > > > > > > > > > I have fixed everything you have pointed out. Diff can be found below. > > > > > LGTM > > > > > > 10.02.2020 14:18, Nikita Pettik пишет: > > > > > > > On 10 Feb 10:47, Olga Arkhangelskaia wrote: > > > > > >  diff --git a/src/box/index.cc b/src/box/index.cc > > > > > > -index 4e4867118..c2fc00867 100644 > > > > > > +index 4e4867118..750838bef 100644 > > > > > >  --- a/src/box/index.cc > > > > > >  +++ b/src/box/index.cc > > > > > > -@@ -733,6 +733,13 @@ int > > > > > > +@@ -733,6 +733,8 @@ int > > > > > >   generic_index_build_next(struct index *index, struct tuple *tuple) > > > > > > { > > > > > >         struct tuple *unused; > > > > > > -+ /* > > > > > > -+       * Note this is not no-op call in case of rtee index: > > > > > > -+       * reserving 0 bytes is required during rtree recovery. > > > > > > -+       * For details see memtx_rtree_index_reserve(). > > > > > > -+ */ > > > > > >  +      if (index_reserve(index, 0) != 0) > > > > > >  +              return -1; > > > > > > > > > > > > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c > > > > > > -index 8badad797..612fcb2a9 100644 > > > > > > +index 8badad797..a5cae8f45 100644 > > > > > >  --- a/src/box/memtx_rtree.c > > > > > >  +++ b/src/box/memtx_rtree.c > > > > > > -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct > > > > > > tuple *old_tuple, > > > > > > +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct > > > > > > tuple *old_tuple, > > > > > >         return 0; > > > > > > } > > > > > > > > > > > > @@ -82,14 +109,13 @@ > > > > > >  +memtx_rtree_index_reserve(struct index *base, uint32_t size_hint) > > > > > >  +{ > > > > > >  + /* > > > > > > -+         * In case of rtree we use reserve to make sure that > > > > > > -+         * memory allocation will not fail during any operation > > > > > > -+         * on rtree, because there is no error handling in the > > > > > > -+         * rtree lib. > > > > > > -+ */ > > > > > > ++       * In case of rtree we use reserve to make sure that memory > > > > > > allocation > > > > > > ++       * will not fail durig any operationin rtree, because there is no > > > > > > ++       * error handling in rtree lib. > > > > > > ++ */ > > > > > >  + (void)size_hint; > > > > > >  +      ERROR_INJECT(ERRINJ_INDEX_RESERVE, { > > > > > > -+              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "mempool", "new > > > > > > slab"); > > > > > > ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator", > > > > > > "memtx extent"); > > > > > >  +              return -1; > > > > > >  + }); > > > > > >  +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine; > > > > > > > > > > > >  diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua > > > > > > -index 03c088677..63c8b19fe 100644 > > > > > > +index 03c088677..598e7da33 100644 > > > > > >  --- a/test/box/errinj.test.lua > > > > > >  +++ b/test/box/errinj.test.lua > > > > > > -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5) > > > > > > +@@ -620,3 +620,30 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5) > > > > > > box.error.injection.get('ERRINJ_RELAY_TIMEOUT') > > > > > >   box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0) > > > > > > box.error.injection.get('ERRINJ_RELAY_TIMEOUT') > > > > > >  + > > > > > >  +-- > > > > > > -+-- gh-4619: make sure that if OOM takes place during rtree recovery, > > > > > > -+-- Tarantool instance will fail gracefully. > > > > > > ++-- gh-4619-RTREE-doesn't-handle-OOM-properly > > > > > >  +-- > > > > > >  +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"') > > > > > >  +test_run:cmd("start server rtree")