[tarantool-patches] Re: [PATCH 02/10] vinyl: add a separate thread for vylog

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev at gmail.com> [19/05/17 17:54]:
> Historically, we use the WAL thread for writing vylog files, because,
> I guess, we didn't want to introduce a separate thread. However, that
> design decision turned out to be quite dubious:

>  - vy_log (vy_log.c) calls vy_log_writer (wal.c) while vy_log_writer
>    calls back to vy_log. That is we have a piece of logic split crudely
>    between two files, which makes the code difficult to follow and just
>    looks ugly.

This is because we can not ship arbitrary logic into a thread
without the thread becoming aware of it. This can be easily fixed
with run_in_cord() function, which would take a cord pointer and a 
fiber function pointer with context, and run the fiber in a given
cord. A mature implementation would take more than just a function
pointer and a context, but some sort of runnable object, which
responds to cancel, exit, suspend and resume. This would be very
useful in a bunch of other places, not just for vy_log.

>  - We can't make vy_log part of vy_env because of this relationship.
>    In fact, vy_log is the last singleton in the whole vy implementation:
>    everything else is wrapped neatly (well, not quite, but still) in
>    vy_env struct.

See above. The wal thread doesn't have to know anything about
struct vy_log or struct wal, for that matter, it's just a
container of runnable objects. We should also be able to move any
runnable object along with its context from a thread to thread
by suspending it first and then resuming at another thread, as I
described.

> 
>  - We can't kill the infamous vy_log.latch, which is a prerequisite for
>    transactional DDL. The latch is needed to sync between vy_log readers
>    and writers. You see, currently we can't read vy_log in the same
>    thread where we write it, which would eliminate the need for any kind
>    of synchronization, because vy_log read is quite a heavy operation -
>    it may stall WAL writes and thus badly affect latency. So we have to
>    do it from a coio thread.

This would be the main reason for the change, however, I would
rather ensure vy_log readers and writers use entirely different
objects as contexts. We already have vy_recovery as vy log reader
context, what prevents you from populating it using an own
xlog object?

As discussed verbally, there is a way to shift entire vy_log
contents to the existing WAL log which would make our future life
much easier. 

Please feel free to accept the above comments and complete this
patch or switch to an entirely new approach (which may well
require the same housekeeping changes I am pushing for in this
review).

-- 
Konstantin Osipov, Moscow, Russia