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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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