Re: [Tarantool-patches] [PATCH] Move rollback to savepoint from ffi to C-API

[Alexander Turenko <alexander.turenko@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 <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{}
>  ---
>