Re: [Tarantool-patches] [PATCH] Move rollback to savepoint from ffi to C-API

[Alexander Turenko <alexander.turenko@tarantool.org>]

We agreed on the following wording with Igor and Leonid:

commit 34234427a3cc1f181ef6583c36ed7e4193bafde7
Author: Leonid Vasiliev <lvasiliev@tarantool.org>
Date:   Fri Dec 13 10:49:33 2019 +0300

    box: rewrite rollback to savepoint to Lua/C
    
    LuaJIT records traces while interpreting Lua bytecode (considering it's
    hot enough) in order to compile the corresponding execution flow to a
    machine code. A Lua/C call aborts trace recording, but an FFI call does
    not abort it per se. If code inside an FFI call yields to another fiber
    while recording a trace and the new current fiber interpreting a Lua
    bytecode too, then unrelated instructions will be recorded to the
    current trace.
    
    In short, we should not yield a current fiber inside an FFI call.
    
    There is another problem. Machine code of a compiled trace may sink a
    value from a Lua state down to a host register, change it and write back
    only at trace exit. So the interpreter state may be outdated during the
    compiled trace execution. A Lua/C call aborts a trace and so the code
    inside a callee always see an actual interpreter state. An FFI call
    however can be turned into a single machine's CALL instruction in the
    compiled code and if the callee accesses a Lua state, then it may see an
    irrelevant value.
    
    In short, we should not access a Lua state directly or reenter to the
    interpreter from an FFI call.
    
    The box.rollback_to_savepoint() function may yield and another fiber
    will be scheduled for execution. If this fiber touches a Lua state, then
    it may see an inconsistent state and the behaviour will be undefined.
    
    Noted that <struct txn>.id starts from 1, because we lean on this fact
    to use luaL_toint64(), which does not distinguish an unexpected Lua type
    and cdata<int64_t> with zero value. It seems that this assumption
    already exists: the code that prepare arguments for 'on_commit' triggers
    uses luaL_toint64() too (see lbox_txn_pairs()).
    
    Fixes #4427
    
    Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
    Reviewed-by: Igor Munkin <imun@tarantool.org>

Pushed to master, 2.3, 2.2 and 1.10.

CCed Kirill.

WBR, Alexander Turenko.

On Mon, Jan 27, 2020 at 10:24:18AM +0300, Leonid Vasiliev wrote:
> The result patch:
> 
> https://github.com/tarantool/tarantool/issues/4427
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rollback-from-ffi-to-c-api-v2
> 
>  box: rewrite rollback to savepoint to Lua/C
> 
>     The fix is prevent an emerge of "FFI sandwich".
>     See
> https://github.com/tarantool/tarantool/issues/4427#issuecomment-524798109
>     for more information.
> 
>     One of the aims of this change (in addition to the fix) is to increase
> locality of highly
>     coupled code for the rollback to savepoint.
> 
>     Noted that <struct txn>.id starts from 1, because I lean on this fact to
>     use luaL_toint64(), which does not distinguish an unexpected Lua type
>     case and cdata<int64_t> with zero value. It seems that this assumption
>     already exists: the code that prepare arguments for 'on_commit' triggers
>     uses luaL_toint64() too (see lbox_txn_pairs()).
> 
>     Fixes #4427
> 
>     Co-authored-by: Alexander Turenko<alexander.turenko@tarantool.org>
>     Co-authored-by: Leonid Vasiliev<lvasiliev@tarantool.org>