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

[Igor Munkin]

Sasha,

I took a look on both patches and I would like to omit all comments I
provided offline to you and Leonid. I just mention that I see no cons in
the second approach therefore it's more preferable for me.

Feel free to proceed.

On 21.01.20, Alexander Turenko wrote:
> 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."
> > 

<snipped>

> > 
> > 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{}
> >  ---
> > 

-- 
Best regards,
IM