From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 64FED46970E for ; Fri, 24 Jan 2020 17:37:32 +0300 (MSK) Date: Fri, 24 Jan 2020 17:35:15 +0300 From: Igor Munkin Message-ID: <20200124143515.GH26983@tarantool.org> References: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> <9b6e89a0-33f3-6844-4a13-b30303c23f36@tarantool.org> <20200121023332.2gyph5jbr2exm2fs@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200121023332.2gyph5jbr2exm2fs@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH] Move rollback to savepoint from ffi to C-API List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Sasha, I took a look on both patches and I would like to omit all comments I provided offline to you and Leonid. I just mention that I see no cons in the second approach therefore it's more preferable for me. Feel free to proceed. On 21.01.20, Alexander Turenko wrote: > CCed Igor. > > Igor, please, look which approach seems more elegant for you. One way > provides more code locality in working with Lua savepoint objects (Lua > tables): it is within one file. Another one provides more locality in > box.rollback_to_savepoint() code, but spreads working with Lua > savepoints across Lua and Lua/C. > > AFAIS, the branch is > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rollback-from-ffi-to-c-api-v2 > > WBR, Alexander Turenko. > > On Tue, Jan 21, 2020 at 12:17:17AM +0300, Leonid Vasiliev wrote: > > We have two possible solution of the FFI sandwich problem with rollback to > > savepoint (with some pros and cons). > > After a conversation with Alexander the next proposal was elaborated: > > "Arbitrator (Igor for example) will pick one of them." > > > > > > Second: > > pros: > > - the lua box.rollback_to_savepoint decorator is absent > > cons: > > - the lua savepoint table ({ csavepoint=csavepoint, > > txn_id=builtin.box_txn_id() }) have defined in Lua and using in C part > > > > > > diff --git a/src/box/lua/init.c b/src/box/lua/init.c > > index 7ffed409d..620a10600 100644 > > --- a/src/box/lua/init.c > > +++ b/src/box/lua/init.c > > @@ -63,6 +63,8 @@ > > #include "box/lua/key_def.h" > > #include "box/lua/merger.h" > > > > +static uint32_t CTID_STRUCT_TXN_SAVEPOINT_PTR = 0; > > + > > extern char session_lua[], > > tuple_lua[], > > key_def_lua[], > > @@ -107,6 +109,101 @@ lbox_rollback(lua_State *L) > > return 0; > > } > > > > +/** > > + * Extract from a cdata value on the Lua > > + * stack. > > + * > > + * The function is a helper for extracting 'csavepoint' field from > > + * a Lua table created using box.savepoint(). > > + */ > > +static struct txn_savepoint * > > +luaT_check_txn_savepoint_cdata(struct lua_State *L, int idx) > > +{ > > + if (lua_type(L, idx) != LUA_TCDATA) > > + return NULL; > > + > > + uint32_t cdata_type; > > + struct txn_savepoint **svp_ptr = luaL_checkcdata(L, idx, &cdata_type); > > + if (svp_ptr == NULL || cdata_type != CTID_STRUCT_TXN_SAVEPOINT_PTR) > > + return NULL; > > + return *svp_ptr; > > +} > > + > > +/** > > + * Extract a savepoint from the Lua stack. > > + * > > + * Expected a value that is created using box.savepoint(): > > + * > > + * { > > + * csavepoint = >, > > + * txn_id = >, > > + * } > > + */ > > +static struct txn_savepoint * > > +luaT_check_txn_savepoint(struct lua_State *L, int idx, int64_t > > *svp_txn_id_ptr) > > +{ > > + /* Verify passed value type. */ > > + if (lua_type(L, idx) != LUA_TTABLE) > > + return NULL; > > + > > + /* Extract and verify csavepoint. */ > > + lua_getfield(L, idx, "csavepoint"); > > + struct txn_savepoint *svp = luaT_check_txn_savepoint_cdata(L, -1); > > + lua_pop(L, 1); > > + if (svp == NULL) > > + return NULL; > > + > > + /* Extract and verify transaction id from savepoint. */ > > + lua_getfield(L, idx, "txn_id"); > > + int64_t svp_txn_id = luaL_toint64(L, -1); > > + lua_pop(L, 1); > > + if (svp_txn_id == 0) > > + return NULL; > > + *svp_txn_id_ptr = svp_txn_id; > > + > > + return svp; > > +} > > + > > +/** > > + * Rollback to a savepoint. > > + * > > + * At success push nothing to the Lua stack. > > + * > > + * At any error raise a Lua error. > > + */ > > +static int > > +lbox_rollback_to_savepoint(struct lua_State *L) > > +{ > > + int64_t svp_txn_id; > > + struct txn_savepoint *svp; > > + > > + if (lua_gettop(L) != 1 || > > + (svp = luaT_check_txn_savepoint(L, 1, &svp_txn_id)) == NULL) > > + return luaL_error(L, > > + "Usage: box.rollback_to_savepoint(savepoint)"); > > + > > + /* > > + * Verify that we're in a transaction and that it is the > > + * same transaction as one where the savepoint was > > + * created. > > + */ > > + struct txn *txn = in_txn(); > > + if (txn == NULL || svp_txn_id != txn->id) { > > + diag_set(ClientError, ER_NO_SUCH_SAVEPOINT); > > + return luaT_error(L); > > + } > > + > > + /* > > + * All checks have been passed: try to rollback to the > > + * savepoint. > > + */ > > + int rc = box_txn_rollback_to_savepoint(svp); > > + if (rc != 0) > > + return luaT_error(L); > > + > > + return 0; > > +} > > + > > /** > > * Get a next txn statement from the current transaction. This is > > * a C closure and 2 upvalues should be available: first is a > > @@ -279,6 +376,7 @@ static const struct luaL_Reg boxlib[] = { > > {"on_commit", lbox_on_commit}, > > {"on_rollback", lbox_on_rollback}, > > {"snapshot", lbox_snapshot}, > > + {"rollback_to_savepoint", lbox_rollback_to_savepoint}, > > {NULL, NULL} > > }; > > > > @@ -293,6 +391,10 @@ static const struct luaL_Reg boxlib_backup[] = { > > void > > box_lua_init(struct lua_State *L) > > { > > + luaL_cdef(L, "struct txn_savepoint;"); > > + CTID_STRUCT_TXN_SAVEPOINT_PTR = luaL_ctypeid(L, > > + "struct txn_savepoint*"); > > + > > /* Use luaL_register() to set _G.box */ > > luaL_register(L, "box", boxlib); > > lua_pop(L, 1); > > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > > index e898c3aa6..50c96a335 100644 > > --- a/src/box/lua/schema.lua > > +++ b/src/box/lua/schema.lua > > @@ -77,9 +77,6 @@ ffi.cdef[[ > > box_txn_savepoint_t * > > box_txn_savepoint(); > > > > - int > > - box_txn_rollback_to_savepoint(box_txn_savepoint_t *savepoint); > > - > > struct port_tuple_entry { > > struct port_tuple_entry *next; > > struct tuple *tuple; > > @@ -334,28 +331,6 @@ box.savepoint = function() > > return { csavepoint=csavepoint, txn_id=builtin.box_txn_id() } > > end > > > > -local savepoint_type = ffi.typeof('box_txn_savepoint_t') > > - > > -local function check_savepoint(savepoint) > > - if savepoint == nil or savepoint.txn_id == nil or > > - savepoint.csavepoint == nil or > > - type(tonumber(savepoint.txn_id)) ~= 'number' or > > - type(savepoint.csavepoint) ~= 'cdata' or > > - not ffi.istype(savepoint_type, savepoint.csavepoint) then > > - error("Usage: box.rollback_to_savepoint(savepoint)") > > - end > > -end > > - > > -box.rollback_to_savepoint = function(savepoint) > > - check_savepoint(savepoint) > > - if savepoint.txn_id ~= builtin.box_txn_id() then > > - box.error(box.error.NO_SUCH_SAVEPOINT) > > - end > > - if builtin.box_txn_rollback_to_savepoint(savepoint.csavepoint) == -1 > > then > > - box.error() > > - end > > -end > > - > > local function atomic_tail(status, ...) > > if not status then > > box.rollback() > > @@ -371,7 +346,7 @@ box.atomic = function(fun, ...) > > end > > > > -- box.commit yields, so it's defined as Lua/C binding > > --- box.rollback yields as well > > +-- box.rollback and box.rollback_to_savepoint yields as well > > > > function update_format(format) > > local result = {} > > diff --git a/src/box/txn.h b/src/box/txn.h > > index da12feebf..97d7b5de5 100644 > > --- a/src/box/txn.h > > +++ b/src/box/txn.h > > @@ -166,6 +166,8 @@ struct txn { > > * A sequentially growing transaction id, assigned when > > * a transaction is initiated. Used to identify > > * a transaction after it has possibly been destroyed. > > + * > > + * Valid IDs start from 1. > > */ > > int64_t id; > > /** List of statements in a transaction. */ > > diff --git a/test/engine/savepoint.result b/test/engine/savepoint.result > > index 86a2d0be2..c23645ce6 100644 > > --- a/test/engine/savepoint.result > > +++ b/test/engine/savepoint.result > > @@ -18,7 +18,7 @@ s1 = box.savepoint() > > ... > > box.rollback_to_savepoint(s1) > > --- > > -- error: 'builtin/box/schema.lua: Usage: > > box.rollback_to_savepoint(savepoint)' > > +- error: 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > box.begin() s1 = box.savepoint() > > --- > > @@ -327,27 +327,27 @@ test_run:cmd("setopt delimiter ''"); > > ok1, errmsg1 > > --- > > - false > > -- 'builtin/box/schema.lua: Usage: box.rollback_to_savepoint(savepoint)' > > +- 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > ok2, errmsg2 > > --- > > - false > > -- 'builtin/box/schema.lua: Usage: box.rollback_to_savepoint(savepoint)' > > +- 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > ok3, errmsg3 > > --- > > - false > > -- 'builtin/box/schema.lua: Usage: box.rollback_to_savepoint(savepoint)' > > +- 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > ok4, errmsg4 > > --- > > - false > > -- 'builtin/box/schema.lua: Usage: box.rollback_to_savepoint(savepoint)' > > +- 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > ok5, errmsg5 > > --- > > - false > > -- 'builtin/box/schema.lua: Usage: box.rollback_to_savepoint(savepoint)' > > +- 'Usage: box.rollback_to_savepoint(savepoint)' > > ... > > s:select{} > > --- > > -- Best regards, IM