Re: [Tarantool-patches] [PATCH] vinyl: rotate mem during index build on demand

[Vladislav Shpilevoy <v.shpilevoy@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);

Схематически понятно. Но реальный тест выглядит почему-то сложнее.
Наверное из самого непонятного все еще то, почему дамп не должен
произойти.