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

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

[Leonid Vasiliev]

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
---
 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)");
+
+	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);
+
+	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
 end
-- 
2.17.1

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

[Alexander Turenko]

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

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

[Leonid Vasiliev]

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

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

[Alexander Turenko]

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

Re: [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

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

[Leonid Vasiliev]

The result patch:

https://github.com/tarantool/tarantool/issues/4427
https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rollback-from-ffi-to-c-api-v2

  box: rewrite rollback to savepoint to Lua/C

     The fix is prevent an emerge of "FFI sandwich".
     See 
https://github.com/tarantool/tarantool/issues/4427#issuecomment-524798109
     for more information.

     One of the aims of this change (in addition to the fix) is to 
increase locality of highly
     coupled code for the rollback to savepoint.

     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()).

     Fixes #4427

     Co-authored-by: Alexander Turenko<alexander.turenko@tarantool.org>
     Co-authored-by: Leonid Vasiliev<lvasiliev@tarantool.org>

---

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

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

[Alexander Turenko]

We agreed on the following wording with Igor and Leonid:

commit 34234427a3cc1f181ef6583c36ed7e4193bafde7
Author: Leonid Vasiliev <lvasiliev@tarantool.org>
Date:   Fri Dec 13 10:49:33 2019 +0300

    box: rewrite rollback to savepoint to Lua/C
    
    LuaJIT records traces while interpreting Lua bytecode (considering it's
    hot enough) in order to compile the corresponding execution flow to a
    machine code. A Lua/C call aborts trace recording, but an FFI call does
    not abort it per se. If code inside an FFI call yields to another fiber
    while recording a trace and the new current fiber interpreting a Lua
    bytecode too, then unrelated instructions will be recorded to the
    current trace.
    
    In short, we should not yield a current fiber inside an FFI call.
    
    There is another problem. Machine code of a compiled trace may sink a
    value from a Lua state down to a host register, change it and write back
    only at trace exit. So the interpreter state may be outdated during the
    compiled trace execution. A Lua/C call aborts a trace and so the code
    inside a callee always see an actual interpreter state. An FFI call
    however can be turned into a single machine's CALL instruction in the
    compiled code and if the callee accesses a Lua state, then it may see an
    irrelevant value.
    
    In short, we should not access a Lua state directly or reenter to the
    interpreter from an FFI call.
    
    The box.rollback_to_savepoint() function may yield and another fiber
    will be scheduled for execution. If this fiber touches a Lua state, then
    it may see an inconsistent state and the behaviour will be undefined.
    
    Noted that <struct txn>.id starts from 1, because we lean on this fact
    to use luaL_toint64(), which does not distinguish an unexpected Lua type
    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()).
    
    Fixes #4427
    
    Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
    Reviewed-by: Igor Munkin <imun@tarantool.org>

Pushed to master, 2.3, 2.2 and 1.10.

CCed Kirill.

WBR, Alexander Turenko.

On Mon, Jan 27, 2020 at 10:24:18AM +0300, Leonid Vasiliev wrote:
> The result patch:
> 
> https://github.com/tarantool/tarantool/issues/4427
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4427-move-rollback-from-ffi-to-c-api-v2
> 
>  box: rewrite rollback to savepoint to Lua/C
> 
>     The fix is prevent an emerge of "FFI sandwich".
>     See
> https://github.com/tarantool/tarantool/issues/4427#issuecomment-524798109
>     for more information.
> 
>     One of the aims of this change (in addition to the fix) is to increase
> locality of highly
>     coupled code for the rollback to savepoint.
> 
>     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()).
> 
>     Fixes #4427
> 
>     Co-authored-by: Alexander Turenko<alexander.turenko@tarantool.org>
>     Co-authored-by: Leonid Vasiliev<lvasiliev@tarantool.org>