From: Olga Arkhangelskaia <arkholga@tarantool.org> To: Nikita Pettik <korablev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4] memtx: fix out of memory handling for rtree Date: Tue, 24 Dec 2019 11:31:20 +0300 [thread overview] Message-ID: <3bff95d3-5f72-5188-32ab-a8bfada29e77@tarantool.org> (raw) In-Reply-To: <e23fa6f1-9f22-7e6a-7b1a-00ac8b565b51@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. > >
prev parent reply other threads:[~2019-12-24 8:31 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-20 13:44 Olga Arkhangelskaia 2019-12-20 15:35 ` Nikita Pettik 2019-12-24 8:06 ` Olga Arkhangelskaia 2019-12-24 8:31 ` Olga Arkhangelskaia [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3bff95d3-5f72-5188-32ab-a8bfada29e77@tarantool.org \ --to=arkholga@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4] memtx: fix out of memory handling for rtree' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox