[Tarantool-patches] [PATCH] Move rollback to savepoint from ffi to C-API
Alexander Turenko
alexander.turenko at tarantool.org
Tue Jan 21 05:33:32 MSK 2020
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 <struct txn_savepoint *> 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 = <cdata<struct txn_savepoint *>>,
> + * txn_id = <cdata<int64_t>>,
> + * }
> + */
> +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{}
> ---
>
More information about the Tarantool-patches
mailing list