From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 869C946970E for ; Mon, 27 Jan 2020 10:24:20 +0300 (MSK) References: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> <9b6e89a0-33f3-6844-4a13-b30303c23f36@tarantool.org> <20200121023332.2gyph5jbr2exm2fs@tkn_work_nb> <20200124143515.GH26983@tarantool.org> From: Leonid Vasiliev Message-ID: <851af63f-4ced-c1de-cac8-eebcd0b6dcd3@tarantool.org> Date: Mon, 27 Jan 2020 10:24:18 +0300 MIME-Version: 1.0 In-Reply-To: <20200124143515.GH26983@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Igor Munkin , Alexander Turenko Cc: tarantool-patches@dev.tarantool.org 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 .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()). Fixes #4427 Co-authored-by: Alexander Turenko Co-authored-by: Leonid Vasiliev --- 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{} ---