From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5D61B46970E for ; Mon, 10 Feb 2020 14:18:27 +0300 (MSK) Date: Mon, 10 Feb 2020 14:18:26 +0300 From: Nikita Pettik Message-ID: <20200210111826.GD1110@tarantool.org> References: <20200210074712.86876-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200210074712.86876-1-arkholga@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 On 10 Feb 10:47, Olga Arkhangelskaia wrote: > Changes in v2: > - changed way of error handling, now we reserve pages before go to rtree lib > - changed test > - changed commit msg > > Changes in v3: > - added memtx_rtree_build_next function > - memory reservation is moved to memtx_rtree_build_next > > Changes in v4: > - added index reservation in build_next > - added memtx_rtree_reserve implementation > > Changes in v5: > - added error injection > - test is placed in errorinj.test.lua > - test is much faster now > - fixed indentation errors > > Changes in v6: > - fixed indentation > - fixed test output Bump patch version only in case functional changes are significant enough. Please, do not send new patch in response to the current review, simply inline your answers like "applied/skipped" etc. >From technical point of view patch LGTM. A few suggestions concerning code documentation below (skip them or apply - it is up to you). I guess smb else should look at the patch before it is pushed to master (as a rule 2 LGTMs are enough). > diff --git a/src/box/index.cc b/src/box/index.cc > index 4e4867118..750838bef 100644 > --- a/src/box/index.cc > +++ b/src/box/index.cc > @@ -733,6 +733,8 @@ int > generic_index_build_next(struct index *index, struct tuple *tuple) > { > struct tuple *unused; I'd add a brief comment here: diff --git a/src/box/index.cc b/src/box/index.cc index 750838bef..c2fc00867 100644 --- a/src/box/index.cc +++ b/src/box/index.cc @@ -733,6 +733,11 @@ 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; > + if (index_reserve(index, 0) != 0) > + return -1; > return index_replace(index, NULL, tuple, DUP_INSERT, &unused); > } > > --- a/src/box/memtx_rtree.c > +++ b/src/box/memtx_rtree.c > @@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct tuple *old_tuple, > return 0; > } > > +static int > +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 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, "slab allocator", "memtx extent"); > + return -1; > + }); Consider this small fix: diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c index a5cae8f45..590321a7e 100644 --- a/src/box/memtx_rtree.c +++ b/src/box/memtx_rtree.c @@ -246,13 +246,14 @@ static int 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 durig any operationin rtree, because there is no - * error handling in rtree lib. + * 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. */ (void)size_hint; ERROR_INJECT(ERRINJ_INDEX_RESERVE, { - diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator", "memtx extent"); + diag_set(OutOfMemory, MEMTX_EXTENT_SIZE,"mempool", "new slab"); return -1; }); > diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua > index 03c088677..598e7da33 100644 > --- a/test/box/errinj.test.lua > +++ b/test/box/errinj.test.lua > @@ -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-RTREE-doesn't-handle-OOM-properly > +-- I'd a bit extend this comment like: -- gh-4619: make sure that if OOM takes place during rtree recovery, -- Tarantool instance will fail gracefully. > +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"') > +test_run:cmd("start server rtree") > +test_run:cmd('switch rtree') > +math = require("math")