From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 97C2446970E for ; Fri, 17 Jan 2020 00:17:11 +0300 (MSK) Date: Fri, 17 Jan 2020 00:17:15 +0300 From: Alexander Turenko Message-ID: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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 Sorry for the late response. I propose to rewrite box.rollback_to_savepoint() using Lua/C to improve code locality. See proposed fixups below the email and on the Totktonada/rewrite-rollback-to-savepoint-fixups branch. If you're agree with the proposed changes, then apply and squash them into your commit. Otherwise, let's continue the discussion. I also have several comments, see below. WBR, Alexander Turenko. ---- Cited the whole commit message: > txn: move rollback to savepoint from ffi to C-API Nit: I would just use 'box:' prefix. 'txn' would refer the same named C module (we have it) or the same named Lua module (we have no one). Nit: I would say 'rewrite box.rollback_to_savepoint using Lua/C' (see [1] as source of the term 'Lua/C API') rather then 'move from FFI to Lua/C'. I think 'rewrite' fit better here then 'move'. Nit: FFI is the abbreviation and it is better to write it uppercased. LuaJIT docs do it, see [2]. [1]: http://luajit.org/ext_c_api.html [2]: http://luajit.org/extensions.html#ffi > Let's add a few words about why this commit is needed. Yep, there is the linked issue, but it stated a problem with a flaky test, then contains the discussion and only then the exact problem description. And it is good to have a motivation right in a commit message in general, I think. > Fixes #4427 On Fri, Dec 13, 2019 at 03:08:10PM +0300, Leonid Vasiliev wrote: > https://github.com/tarantool/tarantool/issues/4427 > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rallback-from-ffi-to-c-api-v2 > > After the "FFI vs C-API" discussion > --- Nit: Let's place comments about a patch under `---` mark: this way `git am` would correctly apply the commit w/o this comment. We usually cherry-pick patches from a development branches, but it is good to have an email consistent with a branch. > src/box/lua/init.c | 28 ++++++++++++++++++++++++++++ > src/box/lua/schema.lua | 5 +---- > 2 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/src/box/lua/init.c b/src/box/lua/init.c > index 7ffed409d..3d26dbb93 100644 > --- a/src/box/lua/init.c > +++ b/src/box/lua/init.c > @@ -107,6 +107,26 @@ lbox_rollback(lua_State *L) > return 0; > } > > +static int > +lbox_txn_rollback_to_savepoint(struct lua_State *L) > +{ > + if (lua_gettop(L) != 1 || lua_type(L, 1) != LUA_TCDATA) > + luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)"); 'txn:rollback to savepoint' -> 'box.rollback_to_savepoint(savepoint)' Look how the error is reported currently: tarantool> box.rollback_to_savepoint() --- - error: 'builtin/box/schema.lua:345: Usage: box.rollback_to_savepoint(savepoint)' ... (I made it as part of the first fixup, see below.) > + > + uint32_t cdata_type; > + struct txn_savepoint **sp_ptr = luaL_checkcdata(L, 1, &cdata_type); What if a cdata value with other ctype is passed? I would implement the check in our usual way, see luaT_check_key_def() for example. The only caveat here is that we should use | luaL_ctypeid(L, "struct txn_savepoint*"); rather than | luaL_ctypeid(L, "struct txn_savepoint&"); to acquire exactly same ctype (with the same ctype id) as is generated by ffi.cdef() in schema.lua by LuaJIT C declaration parser. I made it in the first fixup, see below. > + > + if (sp_ptr == NULL) > + luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)"); > + > + int rc = box_txn_rollback_to_savepoint(*sp_ptr); > + if (rc != 0) > + return luaT_push_nil_and_error(L); > + > + lua_pushnumber(L, 0); > + 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,6 +308,11 @@ static const struct luaL_Reg boxlib_backup[] = { > {NULL, NULL} > }; > > +static const struct luaL_Reg boxlib_internal[] = { > + {"rollback_to_savepoint", lbox_txn_rollback_to_savepoint}, > + {NULL, NULL} > +}; > + > #include "say.h" > > void > @@ -300,6 +325,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..b08a8c615 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) == nil then > box.error() > end I would remove this wrapper at all and write all code using Lua/C to improve locality of the code and don't introduce one more function with its own contract. See the second of the proposed fixups. > end > -- > 2.17.1 ---- commit 9e828bbcbedede1f2417eda9c83f78e38d9d7ee3 Author: Alexander Turenko Date: Thu Jan 16 16:34:59 2020 +0300 FIXUP: txn: move rollback to savepoint from ffi to C-API Strengthen box.internal.rollback_to_savepoint() validation. Changed values that the function pushes to te Lua stack: it is either 0 or -1, just like the corresponding C function. It seems to be logical, since an error is returned via a diagnostics area. diff --git a/src/box/lua/init.c b/src/box/lua/init.c index 3d26dbb93..bb8197137 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,23 +109,41 @@ lbox_rollback(lua_State *L) return 0; } -static int -lbox_txn_rollback_to_savepoint(struct lua_State *L) +/** + * Extract a savepoint from the Lua stack. + */ +static struct txn_savepoint * +luaT_check_txn_savepoint(struct lua_State *L, int idx) { - if (lua_gettop(L) != 1 || lua_type(L, 1) != LUA_TCDATA) - luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)"); + if (lua_type(L, idx) != LUA_TCDATA) + return NULL; uint32_t cdata_type; - struct txn_savepoint **sp_ptr = luaL_checkcdata(L, 1, &cdata_type); - - if (sp_ptr == NULL) - luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)"); - - int rc = box_txn_rollback_to_savepoint(*sp_ptr); - if (rc != 0) - return luaT_push_nil_and_error(L); + 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; +} - lua_pushnumber(L, 0); +/** + * 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_txn_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; } @@ -318,6 +338,10 @@ static const struct luaL_Reg boxlib_internal[] = { 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 b08a8c615..1c49531e7 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -348,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 box.internal.rollback_to_savepoint(savepoint.csavepoint) == nil then + if box.internal.rollback_to_savepoint(savepoint.csavepoint) == -1 then box.error() end end commit d0092eaf32a99524e6db559140f30df87afed6b0 Author: Alexander Turenko Date: Thu Jan 16 20:54:27 2020 +0300 FIXUP: txn: move rollback to savepoint from ffi to C-API Fully rewrote box.rollback_to_savepoint() to Lua/C. Before this fixup it was partially written on Lua/C and partially using pure Lua. The main motivation of this change is to increase locality of highly coupled code. Before we have three functions: pure C, Lua/C and pure Lua. They are written within different files and each has its own contract (however the previous fixup make Lua/C function API quite similar to C's one). Now we have only two layers: C function and Lua/C function. Aside of that I changed the name of the Lua/C function: * Was: lbox_txn_rollback_to_savepoint() * Now: lbox_rollback_to_savepoint() It better fit our naming style: a name is based on a Lua function name rather then on a C function name. See lbox_commit() and others. Noted that .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 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()). diff --git a/src/box/lua/init.c b/src/box/lua/init.c index bb8197137..620a10600 100644 --- a/src/box/lua/init.c +++ b/src/box/lua/init.c @@ -110,10 +110,14 @@ lbox_rollback(lua_State *L) } /** - * Extract a savepoint from the Lua stack. + * 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(struct lua_State *L, int idx) +luaT_check_txn_savepoint_cdata(struct lua_State *L, int idx) { if (lua_type(L, idx) != LUA_TCDATA) return NULL; @@ -125,26 +129,79 @@ luaT_check_txn_savepoint(struct lua_State *L, int idx) 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. * - * This is the helper function: it is registered in box.internal - * and is not the same as box.rollback_to_savepoint(). + * At success push nothing to the Lua stack. * - * Push zero at success and -1 at an error. + * At any error raise a Lua error. */ static int -lbox_txn_rollback_to_savepoint(struct lua_State *L) +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)) == NULL) + (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); - lua_pushnumber(L, rc); - return 1; + if (rc != 0) + return luaT_error(L); + + return 0; } /** @@ -319,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} }; @@ -328,11 +386,6 @@ static const struct luaL_Reg boxlib_backup[] = { {NULL, NULL} }; -static const struct luaL_Reg boxlib_internal[] = { - {"rollback_to_savepoint", lbox_txn_rollback_to_savepoint}, - {NULL, NULL} -}; - #include "say.h" void @@ -349,9 +402,6 @@ 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 1c49531e7..50c96a335 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -331,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 box.internal.rollback_to_savepoint(savepoint.csavepoint) == -1 then - box.error() - end -end - local function atomic_tail(status, ...) if not status then box.rollback() @@ -368,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{} ---