From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 798C146970E for ; Tue, 24 Dec 2019 11:31:22 +0300 (MSK) From: Olga Arkhangelskaia References: <20191220134429.34390-1-arkholga@tarantool.org> <20191220153518.GA72709@tarantool.org> Message-ID: <3bff95d3-5f72-5188-32ab-a8bfada29e77@tarantool.org> Date: Tue, 24 Dec 2019 11:31:20 +0300 MIME-Version: 1.0 In-Reply-To: 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 On 24/12/2019 11:06, Olga Arkhangelskaia wrote: > 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. > I have looked closer - it is _(ERRINJ_INDEX_ALLOC, ERRINJ_BOOL actually. > >