Re: [Tarantool-patches] [PATCH v27 2/3] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Jan 14, 2022 at 01:20:27PM +0300, Serge Petrenko wrote:
...
> > Давай по-русски, тут какое-то недопонимание.
> > 
> > В старых версиях Кирилл пытался лочить слишком мелко. Протестировать такое было
> > тяжеловато. Потому та версия не зашла - тестов было 0.
> 
> В общем после обсуждения в чате я понял твою идею.
> В предпоследней версии (26) уже не было таких мелких локов.
> Там лочились функции лимба целиком.

Да, и там вылезало, что некоторые функции реентарные, некоторые переменные
надо было переводить в режим read-safe (например, тестирование того, что
очередь пуста, хотя мы в этот момент могли лок забрать). там куча проблем
повылезала связанная с общей архитектурой. поэтому идею с локами лимба в
той форме, в которой мне хотелось ее увидеть, пришлось отложить.

> > > > I am confused because of this sentence. "Cover everything we don't know" is
> > > > rather opposite to fine-grained locking. I voted for big locks because
> > > > apparently it was too hard to implement smaller more precise locks.
> > > > 
> > > > > Besides, simply locking issue_promote/issue_demote should be
> > > > > much easier than implementing the fine-grained locking patch.
> > > > Yes. I remember the proposal was to lock entire promote/demote and other
> > > > qsync/raft functions from beginning to end. Because it should be relatively
> > > > easy. I didn't look at the code in this patch though, can't comment it.
> > > This particular patch only locks applier_apply_synchro_request(), txn_limbo_process()
> > > and txn_limbo_is_replica_outdated(), so that applier cannot apply a request from an
> > > already stale term.
> > > 
> > > My proposal is to lock box_issue_promote() and box_issue_demote()
> > > (not whole promote/demote) to get rid of another race: when promote is written
> > > to WAL, but not yed processed.
> > Почему не лочить целиком promote/demote? Может если локи были бы шире, то не
> > нужно было бы и на триггеры портировать все как в новом тикете?
> 
> Как ты и написал в чате, залочить их прямо целиком не получится. Там есть
> место, где мы ждём применения CONFIRM/ROLLBACK/etc.
> 
> Лочить какой-то большой кусок промоута (но не весь промоут) я пока смысла не
> вижу. Может получиться что там опять какой-то дедлок будет или ещё что-то, и мы
> будем кучу времени с этим разбираться (непонятно ради чего).
> 
> В общем, Кирилл, я предлагаю в этом патче добавить лок на весь
> box_issue_promote()/demote(). Выглядит так, что это не добавит проблем,
> зато это точно закроет багос с тем, что после отправленного на запись
> PROMOTE апплаер может применить реквест из уже устаревшего терма.

Добро!

> Нужно будет и тест на это сделать, но он выглядит довольно просто:
> сделать на ноде 1 промоут, на ноде 2 сделать
> error.injection.set(ERRINJ_WAL_WRITE_COUNTDOWN, 2)
> (или как там надо чтобы терм записался, а промот завис) box.ctl.promote()
> на ноде 1 что-нибудь вставить в синхронный спейс
> на ноде 2 убрать ерриндж и проверить что вставка в спейс не прошла.
> 
> С тестом на демоут чуть сложнее будет, потому что он только на мастере
> может работать.

Хорошо, попробую. Я пока начал переводить текущйи тест в формат luatest,
ну когда время позволяет.

> > > What you're talking about is what I call "fine grained locking", and it turned
> > > out rather hard to implement, so Cyrill abandoned this idea for now.
> > fine grained значит "мелко-зернистый". То есть локи были бы на мелкие куски кода,
> > как сначала Кирилл пытался сделать. Я как раз за наоборот топлю - блокировать
> > сразу большие куски, а не "мелко".
> 
> Ага, теперь понятно.