Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich <sergos@tarantool.org>
To: Olga Arkhangelskaia <arkholga@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v6] memtx: fix out of memory handling for rtree
Date: Wed, 19 Feb 2020 19:28:43 +0300	[thread overview]
Message-ID: <20200219162843.GA68447@tarantool.org> (raw)
In-Reply-To: <714780e3-1383-62da-94f9-0e90542d47aa@tarantool.org>

Hi!

I am sorry, I took the OKriw/gh-4619-RTREE-doesnt-handle-OOM-properly
branch for review, instead of OKriw/gh-RTREE-doesnt-handle-OOM-properly

I can see memtx_index_extent_reserve() has an error injection already.
Is it not sufficient, so you still need a new one in
memtx_rtree_index_reserve() ?

Otherwise looks good.

Regards,
Sergos


On 18 фев 21:20, Olga Arkhangelskaia wrote:
> Hi! Thanks for the review! But I am a bit confused.
> 
> 18.02.2020 15:46, Sergey Ostanevich пишет:
> > Hi!
> > 
> > Thanks for the patch!
> > 
> > I wonder why do you use additional by-ref argument to pass an integer
> > result instead of redefining the interface into:
> > 
> >      int
> >      rtree_insert(struct rtree *tree, struct rtree_rect *rect, record_t obj)
> > 
> > instead, so that all uses of a form:
> > 
> >      int err = 0;
> >      ...
> >      rtree_insert(&index->tree, &rect, new_tuple, &err);
> 
> 
> There is no way rtree returns an error. Library itself can not fail.  And
> the best way is to reserve pages before malloc itself in the library.
> 
> We have discussed in the very beginning with Konstantin Osipov and later
> with Aleksandr Lyapunov.
> 
> https://lists.tarantool.org/pipermail/tarantool-patches/2019-November/012392.html
> 
> So we reserve memory instead of returning an error.
> 
> >      if (err == 1) {
> >          diag_set(OutOfMemory, sizeof(struct rtree),
> >                  "rtree", "rtree_page");
> >          return -1;
> >      }
> > 
> > into a much simpler:
> > 
> >      if (rtree_insert(&index->tree, &rect, new_tuple, &err)) {
> >          diag_set(OutOfMemory, sizeof(struct rtree),
> >                  "rtree", "rtree_page");
> >          return -1;
> >      }
> > 
> > Sergos
> > 
> > On 11 Feb 15:42, Nikita Pettik wrote:
> > > On 11 Feb 13:51, Olga Arkhangelskaia wrote:
> > > > Hi Nikita! Thanks for the review.
> > > > 
> > > > I have fixed everything you have pointed out. Diff can be found below.
> > > LGTM
> > > > 10.02.2020 14:18, Nikita Pettik пишет:
> > > > > On 10 Feb 10:47, Olga Arkhangelskaia wrote:
> > > >   diff --git a/src/box/index.cc b/src/box/index.cc
> > > > -index 4e4867118..c2fc00867 100644
> > > > +index 4e4867118..750838bef 100644
> > > >   --- a/src/box/index.cc
> > > >   +++ b/src/box/index.cc
> > > > -@@ -733,6 +733,13 @@ int
> > > > +@@ -733,6 +733,8 @@ 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;
> > > > 
> > > > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
> > > > -index 8badad797..612fcb2a9 100644
> > > > +index 8badad797..a5cae8f45 100644
> > > >   --- a/src/box/memtx_rtree.c
> > > >   +++ b/src/box/memtx_rtree.c
> > > > -@@ -242,6 +242,24 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > tuple *old_tuple,
> > > > +@@ -242,6 +242,23 @@ memtx_rtree_index_replace(struct index *base, struct
> > > > tuple *old_tuple,
> > > >          return 0;
> > > > }
> > > > 
> > > > @@ -82,14 +109,13 @@
> > > >   +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 during any operation
> > > > -+         * on rtree, because there is no error handling in the
> > > > -+         * rtree lib.
> > > > -+ */
> > > > ++       * 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, "mempool", "new
> > > > slab");
> > > > ++              diag_set(OutOfMemory, MEMTX_EXTENT_SIZE, "slab allocator",
> > > > "memtx extent");
> > > >   +              return -1;
> > > >   + });
> > > >   +      struct memtx_engine *memtx = (struct memtx_engine *)base->engine;
> > > > 
> > > >   diff --git a/test/box/errinj.test.lua b/test/box/errinj.test.lua
> > > > -index 03c088677..63c8b19fe 100644
> > > > +index 03c088677..598e7da33 100644
> > > >   --- a/test/box/errinj.test.lua
> > > >   +++ b/test/box/errinj.test.lua
> > > > -@@ -620,3 +620,31 @@ box.error.injection.set('ERRINJ_RELAY_TIMEOUT', 0.5)
> > > > +@@ -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: make sure that if OOM takes place during rtree recovery,
> > > > -+-- Tarantool instance will fail gracefully.
> > > > ++-- gh-4619-RTREE-doesn't-handle-OOM-properly
> > > >   +--
> > > >   +test_run:cmd('create server rtree with script = "box/lua/cfg_rtree.lua"')
> > > >   +test_run:cmd("start server rtree")

  reply	other threads:[~2020-02-19 16:28 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10  7:47 Olga Arkhangelskaia
2020-02-10 11:18 ` Nikita Pettik
2020-02-11 10:33   ` Olga Arkhangelskaia
2020-02-11 10:51   ` Olga Arkhangelskaia
2020-02-11 12:42     ` Nikita Pettik
2020-02-18 12:46       ` Sergey Ostanevich
2020-02-18 18:20         ` Olga Arkhangelskaia
2020-02-19 16:28           ` Sergey Ostanevich [this message]
2020-03-03 11:50             ` Olga Arkhangelskaia
2020-03-03 14:17               ` Sergey Ostanevich
2020-03-04 21:18               ` Nikita Pettik
2020-03-10 11:25                 ` Olga Arkhangelskaia
2020-03-10 13:34                   ` Nikita Pettik

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=20200219162843.GA68447@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=arkholga@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v6] 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