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 9355E46970E for ; Fri, 20 Dec 2019 18:35:19 +0300 (MSK) Date: Fri, 20 Dec 2019 18:35:18 +0300 From: Nikita Pettik Message-ID: <20191220153518.GA72709@tarantool.org> References: <20191220134429.34390-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191220134429.34390-1-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4] 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 20 Dec 16:44, Olga Arkhangelskaia wrote: Hello. Below are a few nits concerning code itself. > diff --git a/src/box/index.cc b/src/box/index.cc > index 4e4867118..d0aa62d67 100644 > --- a/src/box/index.cc > +++ b/src/box/index.cc > @@ -733,7 +733,8 @@ int > generic_index_build_next(struct index *index, struct tuple *tuple) > { > struct tuple *unused; > - return index_replace(index, NULL, tuple, DUP_INSERT, &unused); > + int res = index_reserve(index, 0); > + return res == 0 ? index_replace(index, NULL, tuple, DUP_INSERT, &unused) : res; > } Why not: if (index_reserve(index, 0) != 0) return -1; return index_replace(...); > > void > diff --git a/src/box/memtx_engine.h b/src/box/memtx_engine.h > index f562c66df..f6335cc66 100644 > --- a/src/box/memtx_engine.h > +++ b/src/box/memtx_engine.h > @@ -87,6 +87,18 @@ enum memtx_recovery_state { > /** Memtx extents pool, available to statistics. */ > extern struct mempool memtx_index_extent_pool; > > +enum memtx_reserve_extents_num{ ^ missed space > + /** > + * This number is calculated based on the > + * max (realistic) number of insertions > + * a deletion from a B-tree or an R-tree > + * can lead to, and, as a result, the max > + * number of new block allocations. > + */ > + RESERVE_EXTENTS_BEFORE_DELETE = 8, > + RESERVE_EXTENTS_BEFORE_REPLACE = 16 > +}; > + > /** > * The size of the biggest memtx iterator. Used with > * mempool_create. This is the size of the block that will be > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c > index 8badad797..805fdf54c 100644 > --- a/src/box/memtx_rtree.c > +++ b/src/box/memtx_rtree.c > @@ -242,6 +242,19 @@ 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) static int memtx_rtree_index_reserve(...) > +{ > + /** Comments inside functions start from /* and should be restricted with 66 characters. > + * 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. It is just a check not index reservation. > + */ > + (void)size_hint; > + struct memtx_engine *memtx = (struct memtx_engine *)base->engine; > + return memtx_index_extent_reserve(memtx, > + RESERVE_EXTENTS_BEFORE_REPLACE); Looks like broken indentation. > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 6ef84e045..da20b9196 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -103,18 +103,6 @@ memtx_space_replace_no_keys(struct space *space, struct tuple *old_tuple, > return -1; > } > > -enum { > - /** > - * This number is calculated based on the > - * max (realistic) number of insertions > - * a deletion from a B-tree or an R-tree > - * can lead to, and, as a result, the max > - * number of new block allocations. > - */ > - RESERVE_EXTENTS_BEFORE_DELETE = 8, > - RESERVE_EXTENTS_BEFORE_REPLACE = 16 > -}; > - > /** > * A short-cut version of replace() used during bulk load > * from snapshot. I don't dive into reasons why you avoid using error injection in this case, but still we have separate 'long_run' tests (you can specify long_run option in suite.ini config).