From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 B2E2A469710 for ; Wed, 10 Jun 2020 01:34:59 +0300 (MSK) Date: Tue, 9 Jun 2020 22:34:58 +0000 From: Nikita Pettik Message-ID: <20200609223458.GA11347@tarantool.org> References: <7407b62139349ee3904a674490a6222b0c960a1c.1591292549.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] vinyl: rotate mem during index build on demand List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 07 Jun 17:51, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > On 04/06/2020 19:49, Nikita Pettik wrote: > > Meanwhile in 17f6af7dc the similar problem has been fixed, still it may > > appear that in-memory level of secondary index being constructed has > > lagging memory generation (in other words, it's values is less than > > the value of current global generation). Such situation can be achieved > > if yield which is required to check uniqueness of value being inserted > > is too long. In this time gap other space may trigger dump process > > bumping global memory generation counter, but dump itself is still not > > yet scheduled. > > Why is it so important in this bug to trigger the dump, but stop the > dump from actual scheduling? Triggering the dump process bumps global memory generation. After this event each L0 should be rotated before processing further using of lsregion with old generation (another fiber could insert newer tuple using lsregion). > Why all the needed actions, whatever they > are, can't be done at the trigger moment? Rotating all L0s at once may stall execution for a while. It is done later in scheduler's fiber. > And what are these actions, > which are not done at trigger moment, but done at scheduling, and lead > to this bug? It's memory roratiton in vy_task_dump_new(). > > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > > index a90b786a7..19e37947a 100644 > > --- a/src/box/vinyl.c > > +++ b/src/box/vinyl.c > > @@ -4097,6 +4105,12 @@ vy_build_insert_tuple(struct vy_env *env, struct vy_lsm *lsm, > > * Hence, to get right mem (during mem rotation it becomes > > * sealed i.e. read-only) we should fetch it from lsm again. > > */ > > + if (lsm->mem->generation != *lsm->env->p_generation) { > > + if (vy_lsm_rotate_mem(lsm) != 0) { > > + tuple_unref(stmt); > > + return -1; > > + } > > + } > > 1. This check + rotation now are done in 3 places. In all three there > is a comment explaining that again and again. Isn't it time to extract > it into a function? IMHO, check+rotations doesn't worth another one separate wrapper. Moreover, in-place comments always describe situation better than one general comment. > > mem = lsm->mem; > > > > /* Insert the new tuple into the in-memory index. */ > > diff --git a/test/vinyl/gh-5042-dump-during-index-build-2.result b/test/vinyl/gh-5042-dump-during-index-build-2.result > > new file mode 100644 > > index 000000000..a4fbc56ec > > --- /dev/null > > +++ b/test/vinyl/gh-5042-dump-during-index-build-2.result > > @@ -0,0 +1,189 @@ > > +-- test-run result file version 2 > > +test_run = require('test_run').new() > > + | --- > > + | ... > > + > > +test_run:cmd("create server test with script='vinyl/low_quota.lua'") > > + | --- > > + | - true > > + | ... > > +test_run:cmd("start server test with args='13421772'") > > 2. Why is it 13421772? Just borrowed it from vinyl/gh-4810-dump-during-index.build.test.lua This value obtained from empirical research. Using other values leads to unstable test results for some reason. > > +_ = s2:create_index('pk', {parts = {1, 'unsigned'}}) > > + | --- > > + | ... > > + > > +val = 0 > > + | --- > > + | ... > > +PADDING = string.rep('x', 100) > > 3. Why is it 100? Again, current test is just a modification of gh-4810, which in turn is a compilation of patterns used in different vinyl tests. For instance, this padding I took from select_consistency.test.lua In general, padding is used to increase tuple payload, so that less tuples are required to force dump. > > + | --- > > + | ... > > + > > +test_run:cmd("setopt delimiter ';'") > > + | --- > > + | - true > > + | ... > > + > > +function gen_insert(space) > > + pcall(space.insert, space, {val, val, val, val, PADDING}) > > 4. Why is it important to have 4 vals, and not just one? Why is > it in pcall? IDK, it is just a test case. Removed pcall; left only 3 fields - see diff below. > > + val = val + 1 > > +end; > > + | --- > > + | ... > > + > > +function fill_L0_without_dump(space, watermark) > > + while box.stat.vinyl().memory.level0 < watermark do > > + gen_insert(space) > > + end > > 5. Is it possible to check that the dump really didn't happen? > Since the function is called 'without_dump'. Yes. Original test (gh-4810) contains assertion, but I forgot to copy it. Fixed. > > +end; > > + | --- > > + | ... > > + > > +function insert_loop() > > + while not stop do > > 6. Variable 'stop' is not declared here. I propose to declare it > before this function. Ok, fixed (diff below). > > + gen_insert(s1) > > + end > > + ch:put(true) > > +end; > > + | --- > > + | ... > > + > > +function idx_build() > > + _ = s2:create_index('i1', {unique = true, parts = {2, 'unsigned', 3, 'unsigned'}}) > > + ch:put(true) > > 7. ch is not declared anywhere above. It would be better to declare > things before usage. Easier to read. Fixed. > 8. Is it important to use fields {2, 3} and not just {2}? IDK. Only second one part is left. > > +end; > > + | --- > > + | ... > > + > > +dump_watermark = box.cfg.vinyl_memory / 2; > > 9. Why / 2? Is it some internal vinyl formula, that > the dump is triggered, when the memory is half full? Yes, dump watermarks is always >= vinyl_memory/2. Numbers may vary, but for this vinyl_memory value on my local machine first/second dumps are triggered before reaching 2/3 of vinyl_memory. > In this case won't 'fill_L0_without_dump(s2, dump_watermark);' > this line trigger the dump? Even though it is called > 'without_dump'. Oh god, let's rename it to fill_L0(space, size). > > +errinj.set("ERRINJ_VY_INDEX_DUMP", 1); > > + | --- > > + | - ok > > + | ... > > + > > +fiber.sleep(2); > > 10. Is it possible to avoid the fixed timeout? As you know, > having a fixed timeout very often makes the test flaky. I've failed to come up with condition which can be used as indicator of triggered but not yet scheduled dump. 2 seconds seems to be enough to reach quota limit and trigger the dump even on slow machines. In new version I used 1 second - please check if this test will fail on your PC. > Also the test works more than 10 seconds - I always get the > message: > > No output during 10 seconds. Will abort after 120 seconds without output. List of workers not reporting the status: > - 001_vinyl [vinyl/gh-5042-dump-during-index-build-2.test.lua, None] at var/001_vinyl/gh-5042-dump-during-index-build-2.result:162 > > I suggest to try to find a way to speed it up. Here's whole diff: diff --git a/test/vinyl/gh-5042-dump-during-index-build-2.test.lua b/test/vinyl/gh-5042-dump-during-index-build-2.test.lua index 16cdfa668..52170fc76 100644 --- a/test/vinyl/gh-5042-dump-during-index-build-2.test.lua +++ b/test/vinyl/gh-5042-dump-during-index-build-2.test.lua @@ -36,16 +36,28 @@ PADDING = string.rep('x', 100) test_run:cmd("setopt delimiter ';'") function gen_insert(space) - pcall(space.insert, space, {val, val, val, val, PADDING}) + space:insert{val, val, PADDING} val = val + 1 end; -function fill_L0_without_dump(space, watermark) - while box.stat.vinyl().memory.level0 < watermark do +function fill_L0(space, size) + while box.stat.vinyl().memory.level0 < size do gen_insert(space) end end; +dump_watermark = box.cfg.vinyl_memory / 2; +fill_L0(s2, dump_watermark / 2); + +box.snapshot(); + +dump_cnt_before = box.stat.vinyl().scheduler.dump_count; +fill_L0(s1, dump_watermark / 2 * 3); +assert(dump_cnt_before == box.stat.vinyl().scheduler.dump_count); + +stop = false; +ch = fiber.channel(2); + function insert_loop() while not stop do gen_insert(s1) @@ -53,33 +65,17 @@ function insert_loop() ch:put(true) end; -function idx_build() - _ = s2:create_index('i1', {unique = true, parts = {2, 'unsigned', 3, 'unsigned'}}) - ch:put(true) -end; - -dump_watermark = box.cfg.vinyl_memory / 2; -fill_L0_without_dump(s1, dump_watermark / 2); -fill_L0_without_dump(s2, dump_watermark); - -box.snapshot(); - -fill_L0_without_dump(s1, dump_watermark / 2 * 3); - function idx_build() errinj.set("ERRINJ_BUILD_UNIQUE_IDX_DELAY", true) - _ = s2:create_index('i1', {unique = true, parts = {2, 'unsigned', 3, 'unsigned'}}) + _ = s2:create_index('i1', {unique = true, parts = {2, 'unsigned'}}) ch:put(true) end; -stop = false; -ch = fiber.channel(2); - _ = fiber.create(idx_build); -_ = fiber.create(insert_loop, s1); +_ = fiber.create(insert_loop); errinj.set("ERRINJ_VY_INDEX_DUMP", 1); -fiber.sleep(2); +fiber.sleep(1); errinj.set("ERRINJ_BUILD_UNIQUE_IDX_DELAY", false); errinj.set("ERRINJ_VY_INDEX_DUMP", -1); > Overall I didn't understand almost anything in this test, > despite the big comment in the beginning. So I can't > validate whether it is optimal/correct. All I could check > is that it really crashes without the patch. Probably fixing > the comments above, and adding more explanations into the > test context would help. Ок, давай объясню по шагам. Все что идет до этого момента - подготовка. У s2 (по которому мы будем строить вторичный индекс) должны быть таплы на диске; у s1 заполнен уровень L0, но так, чтобы дамп еще не произошел (т.е. чтобы минимальное кол-во вставок после заполнения приводили к дампу). -- Это тупо луп, который вставляет таплы до упора, чтобы стригерить -- дамп. Тут нельзя использовать box.snapshot() так как он ставит -- латч на ддл и мы тогда не сможем запустить построение индекса. -- function insert_loop() while not stop do gen_insert(s1) end ch:put(true) end; -- Перед тем как начать строить индекс, выставляем эту задержку. -- Так как индекс уникальный, то для мы свалимся в чтение рана -- для проверки уникальности тапла. Можно было бы использовать -- ERRINJ_VY_READ_PAGE_DELAY, но тогда может зависнуть параллельный -- инсерт в спейс s1. Поэтому тут мы имитируем делей+илд сразу после -- vy_check_is_unique_secondary() в vy_build_insert_tuple(). -- Таким образом, после выполнения этой функции мы зависнем в -- vy_build_insert_tuple(), которая в свою очередь вызывается -- в главном цикли при построении вторичного индекса (см. vinyl_space_build_index) -- function idx_build() errinj.set("ERRINJ_BUILD_UNIQUE_IDX_DELAY", true) _ = s2:create_index('i1', {unique = true, parts = {2, 'unsigned'}}) ch:put(true) end; _ = fiber.create(idx_build); _ = fiber.create(insert_loop); -- В этот момент у нас будет такое состояние: s2:create_index() -- зависает в vy_build_insert_tuple(); таплы продолжают вставляться -- в s1. В какой-то момент их станет достаточно, чтобы был превышен -- лимит по памяти и сработал дамп (vy_scheduler_trigger_dump). -- В этот момент значение глобальной генерации увеличивается, а -- файбер отвечающий за планировку дампа просыпается. Чтобы не дать -- сделать ротацию вторичного индекса который мы строим (s2:i1), -- мы ставим ERRINJ_VY_INDEX_DUMP. В теории, это может произойти не -- потому что vy_task_dump_new() завершился с ошибкой, а потому что -- файбер не был попросту разбужен в текущее временное окно (то есть -- управление возвращается сразу к построению индекса). -- С другой стороны, ротация других индексов может иметь место -- (например, в vy_tx_write_prepare()). Соответственно, в lsregion -- могут уже появиться блоки с id > s2:i1->generation. -- Задержка же нужна, чтобы insert_loop() успел стригерить дамп. -- errinj.set("ERRINJ_VY_INDEX_DUMP", 1); -- После того, как планировщик не смог или не успел выполнить -- vy_task_dump_new() (в которой происходит ротация памяти), -- мы возращаемся к построению индекса и продолжаем выполнение. -- Как было сказано, глобальная генерация памяти увеличилась, -- и могла произойти аллокация на lsregion с новым id (увеличенным). -- Когда мы попытаемся в vy_build_insert_stmt() аллоцировать память -- со старым id, то сработает ассерт: slab->max_id <= id. -- fiber.sleep(2); errinj.set("ERRINJ_BUILD_UNIQUE_IDX_DELAY", false); errinj.set("ERRINJ_VY_INDEX_DUMP", -1); > I checked all the other similar places and propose you to take > a look at some of them too: > > - vy_build_recover_stmt() calls vy_build_insert_stmt() without > checking mem generation and rotation; Во время рекавери ротации памяти не могут происходить.. vy_point_lookup() может, конечно, илдить, но дампов быть не может. Вот коммент в vinyl_engine_begin_initial_recovery() подтверждающий это: * We can't schedule any background tasks until * local recovery is complete, because they would * disrupt yet to be recovered data stored on disk. * So we don't start the scheduler fiber or enable * memory limit until then. * * This is OK, because during recovery an instance * can't consume more memory than it used before * restart and hence memory dumps are not necessary. > - vy_squash_process() calls vy_lsm_set() without these actions; В теории потенциально тут такая ситуация может произойти (так как сквош происходит в отдельном файбере), но там выделяется память всего под один тапл (в отличие от построения вторичного индекса, где lsregion_alloc зовется для каждого тапла) и поймать такую ситуацию на практике, наверное, очень маловероятно. > - vy_lsm_commit_upsert() calls vy_lsm_set, ditto. Тут тоже в принципе реально: между engine_prepare() и engine_commit() есть илд. При этом дамп может стригериться в конце vinyl_engine_prepare() (хотя и в vy_tx_prepare() есть ротация). Для этого случая сценарий придумать будет проще. В любом случае, это я уже отдельными патчами пошлю.