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

[Alexander Turenko <alexander.turenko@tarantool.org>]

Sorry for the late response.

I propose to rewrite box.rollback_to_savepoint() using Lua/C to improve
code locality. See proposed fixups below the email and on the
Totktonada/rewrite-rollback-to-savepoint-fixups branch.

If you're agree with the proposed changes, then apply and squash them
into your commit. Otherwise, let's continue the discussion.

I also have several comments, see below.

WBR, Alexander Turenko.

----

Cited the whole commit message:

> txn: move rollback to savepoint from ffi to C-API

Nit: I would just use 'box:' prefix. 'txn' would refer the same named C
module (we have it) or the same named Lua module (we have no one).

Nit: I would say 'rewrite box.rollback_to_savepoint using Lua/C' (see
[1] as source of the term 'Lua/C API') rather then 'move from FFI to
Lua/C'. I think 'rewrite' fit better here then 'move'.

Nit: FFI is the abbreviation and it is better to write it uppercased.
LuaJIT docs do it, see [2].

[1]: http://luajit.org/ext_c_api.html
[2]: http://luajit.org/extensions.html#ffi

>

Let's add a few words about why this commit is needed. Yep, there is the
linked issue, but it stated a problem with a flaky test, then contains
the discussion and only then the exact problem description. And it is
good to have a motivation right in a commit message in general, I think.

> Fixes #4427

On Fri, Dec 13, 2019 at 03:08:10PM +0300, Leonid Vasiliev wrote:
> https://github.com/tarantool/tarantool/issues/4427
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rallback-from-ffi-to-c-api-v2
> 
> After the "FFI vs C-API" discussion
> ---

Nit: Let's place comments about a patch under `---` mark: this way `git
am` would correctly apply the commit w/o this comment. We usually
cherry-pick patches from a development branches, but it is good to have
an email consistent with a branch.

>  src/box/lua/init.c     | 28 ++++++++++++++++++++++++++++
>  src/box/lua/schema.lua |  5 +----
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 7ffed409d..3d26dbb93 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -107,6 +107,26 @@ lbox_rollback(lua_State *L)
>  	return 0;
>  }
>  
> +static int
> +lbox_txn_rollback_to_savepoint(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1 || lua_type(L, 1) != LUA_TCDATA)
> +		luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)");

'txn:rollback to savepoint' -> 'box.rollback_to_savepoint(savepoint)'

Look how the error is reported currently:

tarantool> box.rollback_to_savepoint()
---
- error: 'builtin/box/schema.lua:345: Usage: box.rollback_to_savepoint(savepoint)'
...

(I made it as part of the first fixup, see below.)

> +
> +	uint32_t cdata_type;
> +	struct txn_savepoint **sp_ptr = luaL_checkcdata(L, 1, &cdata_type);

What if a cdata value with other ctype is passed? I would implement the
check in our usual way, see luaT_check_key_def() for example. The only
caveat here is that we should use

 | luaL_ctypeid(L, "struct txn_savepoint*");

rather than

 | luaL_ctypeid(L, "struct txn_savepoint&");

to acquire exactly same ctype (with the same ctype id) as is generated
by ffi.cdef() in schema.lua by LuaJIT C declaration parser.

I made it in the first fixup, see below.

> +
> +	if (sp_ptr == NULL)
> +		luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)");
> +
> +	int rc = box_txn_rollback_to_savepoint(*sp_ptr);
> +	if (rc != 0)
> +		return luaT_push_nil_and_error(L);
> +
> +	lua_pushnumber(L, 0);
> +	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,6 +308,11 @@ static const struct luaL_Reg boxlib_backup[] = {
>  	{NULL, NULL}
>  };
>  
> +static const struct luaL_Reg boxlib_internal[] = {
> +	{"rollback_to_savepoint", lbox_txn_rollback_to_savepoint},
> +	{NULL, NULL}
> +};
> +
>  #include "say.h"
>  
>  void
> @@ -300,6 +325,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..b08a8c615 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) == nil then
>          box.error()
>      end

I would remove this wrapper at all and write all code using Lua/C to
improve locality of the code and don't introduce one more function with
its own contract.

See the second of the proposed fixups.

>  end
> -- 
> 2.17.1

----

commit 9e828bbcbedede1f2417eda9c83f78e38d9d7ee3
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Thu Jan 16 16:34:59 2020 +0300

    FIXUP: txn: move rollback to savepoint from ffi to C-API
    
    Strengthen box.internal.rollback_to_savepoint() validation.
    
    Changed values that the function pushes to te Lua stack: it is either 0
    or -1, just like the corresponding C function. It seems to be logical,
    since an error is returned via a diagnostics area.

diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 3d26dbb93..bb8197137 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,23 +109,41 @@ lbox_rollback(lua_State *L)
 	return 0;
 }
 
-static int
-lbox_txn_rollback_to_savepoint(struct lua_State *L)
+/**
+ * Extract a savepoint from the Lua stack.
+ */
+static struct txn_savepoint *
+luaT_check_txn_savepoint(struct lua_State *L, int idx)
 {
-	if (lua_gettop(L) != 1 || lua_type(L, 1) != LUA_TCDATA)
-		luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)");
+	if (lua_type(L, idx) != LUA_TCDATA)
+		return NULL;
 
 	uint32_t cdata_type;
-	struct txn_savepoint **sp_ptr = luaL_checkcdata(L, 1, &cdata_type);
-
-	if (sp_ptr == NULL)
-		luaL_error(L, "Usage: txn:rollback to savepoint(savepoint)");
-
-	int rc = box_txn_rollback_to_savepoint(*sp_ptr);
-	if (rc != 0)
-		return luaT_push_nil_and_error(L);
+	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;
+}
 
-	lua_pushnumber(L, 0);
+/**
+ * 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_txn_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;
 }
 
@@ -318,6 +338,10 @@ static const struct luaL_Reg boxlib_internal[] = {
 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 b08a8c615..1c49531e7 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -348,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 box.internal.rollback_to_savepoint(savepoint.csavepoint) == nil then
+    if box.internal.rollback_to_savepoint(savepoint.csavepoint) == -1 then
         box.error()
     end
 end

commit d0092eaf32a99524e6db559140f30df87afed6b0
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Thu Jan 16 20:54:27 2020 +0300

    FIXUP: txn: move rollback to savepoint from ffi to C-API
    
    Fully rewrote box.rollback_to_savepoint() to Lua/C. Before this fixup it
    was partially written on Lua/C and partially using pure Lua.
    
    The main motivation of this change is to increase locality of highly
    coupled code. Before we have three functions: pure C, Lua/C and pure
    Lua. They are written within different files and each has its own
    contract (however the previous fixup make Lua/C function API quite
    similar to C's one). Now we have only two layers: C function and Lua/C
    function.
    
    Aside of that I changed the name of the Lua/C function:
    
    * Was: lbox_txn_rollback_to_savepoint()
    * Now: lbox_rollback_to_savepoint()
    
    It better fit our naming style: a name is based on a Lua function name
    rather then on a C function name. See lbox_commit() and others.
    
    Noted that <struct txn>.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<int64_t> 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()).

diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index bb8197137..620a10600 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -110,10 +110,14 @@ lbox_rollback(lua_State *L)
 }
 
 /**
- * Extract a savepoint from the Lua stack.
+ * 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(struct lua_State *L, int idx)
+luaT_check_txn_savepoint_cdata(struct lua_State *L, int idx)
 {
 	if (lua_type(L, idx) != LUA_TCDATA)
 		return NULL;
@@ -125,26 +129,79 @@ luaT_check_txn_savepoint(struct lua_State *L, int idx)
 	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.
  *
- * This is the helper function: it is registered in box.internal
- * and is not the same as box.rollback_to_savepoint().
+ * At success push nothing to the Lua stack.
  *
- * Push zero at success and -1 at an error.
+ * At any error raise a Lua error.
  */
 static int
-lbox_txn_rollback_to_savepoint(struct lua_State *L)
+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)) == NULL)
+	    (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);
-	lua_pushnumber(L, rc);
-	return 1;
+	if (rc != 0)
+		return luaT_error(L);
+
+	return 0;
 }
 
 /**
@@ -319,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}
 };
 
@@ -328,11 +386,6 @@ static const struct luaL_Reg boxlib_backup[] = {
 	{NULL, NULL}
 };
 
-static const struct luaL_Reg boxlib_internal[] = {
-	{"rollback_to_savepoint", lbox_txn_rollback_to_savepoint},
-	{NULL, NULL}
-};
-
 #include "say.h"
 
 void
@@ -349,9 +402,6 @@ 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 1c49531e7..50c96a335 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -331,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 box.internal.rollback_to_savepoint(savepoint.csavepoint) == -1 then
-        box.error()
-    end
-end
-
 local function atomic_tail(status, ...)
     if not status then
         box.rollback()
@@ -368,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{}
 ---