From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 4E6D546970E for ; Tue, 21 Jan 2020 05:33:27 +0300 (MSK) Date: Tue, 21 Jan 2020 05:33:32 +0300 From: Alexander Turenko Message-ID: <20200121023332.2gyph5jbr2exm2fs@tkn_work_nb> References: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> <9b6e89a0-33f3-6844-4a13-b30303c23f36@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9b6e89a0-33f3-6844-4a13-b30303c23f36@tarantool.org> 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: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org 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." > > First: > pros: > - make the fewest possible changes > - the lua savepoint table ({ csavepoint=csavepoint, > txn_id=builtin.box_txn_id() }) is used only in Lua (defenition and using are > ) > cons: > - box.rollback_to_savepoint have some lua decorator > > > diff --git a/src/box/lua/init.c b/src/box/lua/init.c > index 7ffed409d..6d9f23681 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,44 @@ lbox_rollback(lua_State *L) > return 0; > } > > +/** > + * Extract a savepoint from the Lua stack. > + */ > +static struct txn_savepoint * > +luaT_check_txn_savepoint(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; > +} > + > +/** > + * Rollback to a savepoint. > + * > + * This is the helper function: it is registered in box.internal > + * and is not the same as box.rollback_to_savepoint(). > + * > + * Push zero at success and -1 at an error. > + */ > +static int > +lbox_rollback_to_savepoint(struct lua_State *L) > +{ > + struct txn_savepoint *svp; > + if (lua_gettop(L) != 1 || > + (svp = luaT_check_txn_savepoint(L, 1)) == NULL) > + return luaL_error(L, > + "Usage: box.rollback_to_savepoint(savepoint)"); > + > + int rc = box_txn_rollback_to_savepoint(svp); > + lua_pushnumber(L, rc); > + return 1; > +} > + > /** > * Get a next txn statement from the current transaction. This is > * a C closure and 2 upvalues should be available: first is a > @@ -288,11 +328,20 @@ static const struct luaL_Reg boxlib_backup[] = { > {NULL, NULL} > }; > > +static const struct luaL_Reg boxlib_internal[] = { > + {"rollback_to_savepoint", lbox_rollback_to_savepoint}, > + {NULL, NULL} > +}; > + > #include "say.h" > > 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); > @@ -300,6 +349,9 @@ box_lua_init(struct lua_State *L) > luaL_register(L, "box.backup", boxlib_backup); > lua_pop(L, 1); > > + luaL_register(L, "box.internal", boxlib_internal); > + lua_pop(L, 1); > + > box_lua_error_init(L); > box_lua_tuple_init(L); > box_lua_call_init(L); > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index e898c3aa6..1c49531e7 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; > @@ -351,7 +348,7 @@ box.rollback_to_savepoint = function(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 > + if box.internal.rollback_to_savepoint(savepoint.csavepoint) == -1 then > box.error() > end > end > 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. */ > > > 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{} > --- >