From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (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 70522445320 for ; Thu, 23 Jul 2020 01:54:12 +0300 (MSK) References: <7407b62139349ee3904a674490a6222b0c960a1c.1591292549.git.korablev@tarantool.org> <20200609223458.GA11347@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8d6f5fc3-08fc-a121-1235-e133e6cb6425@tarantool.org> Date: Thu, 23 Jul 2020 00:54:09 +0200 MIME-Version: 1.0 In-Reply-To: <20200609223458.GA11347@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Привет! On 10.06.2020 00:34, Nikita Pettik wrote: > 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(). > >>> 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. Звучит не очень хорошо. Есть какая-то константа волшебная, без которой все плохо, и с которой все норм, но непонятно, почему. Надо бы выяснить. >>> +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. Фиксированный таймаут порядка единиц секунд - это всегда потенциально нестабильный тест. Отловить любое событие можно через новый errinj, если использовать его не как вставку ошибки, а как сигнал. См ERRINJ_DYN_MODULE_COUNT для примера. Можно увеличивать счетчик зашедуленых дампов в каком-нибудь errinj, и в тесте его поллить, пока он не увеличится. > @@ -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); Схематически понятно. Но реальный тест выглядит почему-то сложнее. Наверное из самого непонятного все еще то, почему дамп не должен произойти.