[Tarantool-patches] [PATCH] Move rollback to savepoint from ffi to C-API
Alexander Turenko
alexander.turenko at tarantool.org
Fri Jan 17 00:17:15 MSK 2020
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 at 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 at 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{}
---
More information about the Tarantool-patches
mailing list