From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 9931B46970E for ; Tue, 21 Jan 2020 00:17:19 +0300 (MSK) References: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> From: Leonid Vasiliev Message-ID: <9b6e89a0-33f3-6844-4a13-b30303c23f36@tarantool.org> Date: Tue, 21 Jan 2020 00:17:17 +0300 MIME-Version: 1.0 In-Reply-To: <20200116211714.lhwqjfm3o4nhctf7@tkn_work_nb> 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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org 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{} ---