From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 D5F9C46970E for ; Tue, 24 Dec 2019 11:06:05 +0300 (MSK) References: <20191220134429.34390-1-arkholga@tarantool.org> <20191220153518.GA72709@tarantool.org> From: Olga Arkhangelskaia Message-ID: Date: Tue, 24 Dec 2019 11:06:03 +0300 MIME-Version: 1.0 In-Reply-To: <20191220153518.GA72709@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi Nikita, thanks for review. On 20/12/2019 18:35, Nikita Pettik wrote: > 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(...); Ok, I will change. I guess it did't come on my mind. > >> >> 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. Will fix, and all above too. > >> + * 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. It is likely, I have change my work machine and some options may have been missed, will pay more attention. >> 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). Actually it is long on my mac =)) However, I am not sure what to use 84     _(ERRINJ_INDEX_ALLOC, ERRINJ_BOOL 85     _(ERRINJ_TUPLE_ALLOC, ERRINJ_BOOL because it is extent allocation that fail. Will be glad to hear what should be chosen. >