Re: [Tarantool-patches] [PATCH v5 3/5] box/applier: fix nil dereference in applier rollback

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Cyrill Gorcunov <gorcunov@gmail.com> [20/02/05 11:31]:
> > I don't understand this comment. How can it be lost exactly?
> 
> Hmmm, I think you're right. Actually unweaving the all possible
> call traces by hands (which I had to do) is quite exhausting task
> so I might be wrong here.

It was added by parallel applier patch, so the most likely cause
of the bug is that this way of cancelling parallel appliers on
conflict was not tested well enough. Previously we only had a
single applier per peer so did not need to coordinate.

> > Let's begin by explaining why we need to cancel the reader fiber here.
> 
> This fiber_cancel has been already here, I only added diag_set(FiberIsCancelled)
> to throw an exception thus the caller would zap this applier fiber.
> Actually I think we could retry instead of reaping off the fiber
> completely but it requies more deep understanding of how applier
> works. So I left it in comment.

No, we can't and shouldn't retry. Retry handling is done elsewhere already - see replication_skip_conflict.

If replication is stopped by an apply error, it's most likely a
transaction conflict, indicating that active-active setup is
broken, so it has to be resolved by a DBA (which can set
replication_skip_conflict). This is why it's critical to preserve
the original error.
> 
> Not exactly, if I understand the initial logic of this applier
> try/cath branch  we need to setup replicaset.applier.diag and
> then on FiberIsCancelled we should move it from replicaset.applier.diag
> back to current fiber->diag.

Please dig into what is "current" here. Which fiber is current if
there are many fibers handling a single peer?

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com