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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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