* [tarantool-patches] Re: [PATCH] initial version: remove savepoints from FFI
2019-09-02 11:14 [tarantool-patches] [PATCH] initial version: remove savepoints from FFI sergos
@ 2019-09-09 12:06 ` Alexander Turenko
0 siblings, 0 replies; 2+ messages in thread
From: Alexander Turenko @ 2019-09-09 12:06 UTC (permalink / raw)
To: sergos; +Cc: tarantool-patches
I think it worth to extract all related function into its own
box/lua/txn.{c,h,lua} module. As I see the relevant functions are:
* box.begin()
* box.is_in_txn()
* box.savepoint()
* box.rollback_to_savepoint()
* box.atomic()
All calls into C should be performed via Lua-C API (I only doubt about
is_in_txn() -- will ffi be a way performant?).
box.atomic() is just simple wrapper, can be implemented in pure Lua
(box/lua/txn.lua). Other functions can be pure C (box/lua/txn.c).
Don't sure why struct txn_savepoint cdata is wrapped into a table with
txn id. Maybe it worth to add id into struct txn_savepoint? Need to
investigate.
See more comments below.
WBR, Alexander Turenko.
On Mon, Sep 02, 2019 at 11:14:14AM +0000, sergos@tarantool.org wrote:
> From: Sergey Ostanevich <sergos@tarantool.org>
>
> fixes: #4427
>
> follow-up: need to remove savepoints from FFI completely
> ---
Please, write here at least a link to an issue and a link to a branch.
Example:
https://www.freelists.org/post/tarantool-patches/PATCH-v1-11-lua-cjson-fix-segfault-on-recursive-table-encoding
> src/box/CMakeLists.txt | 1 +
> src/box/lua/init.c | 2 +
> src/box/lua/schema.lua | 6 +--
> src/box/lua/txn.c | 90 ++++++++++++++++++++++++++++++++++++++++++
> src/box/lua/txn.h | 49 +++++++++++++++++++++++
> src/box/txn.c | 2 +
> 6 files changed, 146 insertions(+), 4 deletions(-)
> create mode 100644 src/box/lua/txn.c
> create mode 100644 src/box/lua/txn.h
>
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 9bba37bcb..27a0902f1 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -142,6 +142,7 @@ add_library(box STATIC
> lua/info.c
> lua/stat.c
> lua/ctl.c
> + lua/txn.c
All CI jobs are failed:
[069] Test failed! Result content mismatch:
[069] --- box/misc.result Mon Sep 2 11:16:13 2019
[069] +++ box/misc.reject Mon Sep 2 11:18:34 2019
[069] @@ -86,6 +86,7 @@
[069] - space
[069] - stat
[069] - tuple
[069] + - txn
Please, ensure that tests are pass (at least locally) before sending a
patch.
> lua/error.cc
> lua/session.c
> lua/net_box.c
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 7ffed409d..a61821fb4 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -53,6 +53,7 @@
> #include "box/lua/stat.h"
> #include "box/lua/info.h"
> #include "box/lua/ctl.h"
> +#include "box/lua/txn.h"
> #include "box/lua/session.h"
> #include "box/lua/net_box.h"
> #include "box/lua/cfg.h"
> @@ -312,6 +313,7 @@ box_lua_init(struct lua_State *L)
> box_lua_info_init(L);
> box_lua_stat_init(L);
> box_lua_ctl_init(L);
> + box_lua_txn_init(L);
> box_lua_session_init(L);
> box_lua_xlog_init(L);
> box_lua_execute_init(L);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 65017d391..0fcb2b146 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -77,8 +77,6 @@ ffi.cdef[[
> box_txn_savepoint_t *
> box_txn_savepoint();
>
> - int
> - box_txn_rollback_to_savepoint(box_txn_savepoint_t *savepoint);
It worth to remove this function from exports so (see extra/exports). It
also marked with API_EXPORT macro: don't sure what it is, maybe it is
not more needed too.
>
> struct port_tuple_entry {
> struct port_tuple_entry *next;
> @@ -351,8 +349,8 @@ 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
> - box.error()
> + if box.txn.rollback_to_savepoint(savepoint) == -1 then
> + box.error(box.error.NO_SUCH_FUNCTION, "box.txn.rollback_to_savepoint")
box_txn_rollback_to_savepoint() (from box/txn.c) returns -1 in case of
an error and set a diag. In Lua we did raise the error that is on top of
the diagnostic area.
Now we call other function, box.txn.rollback_to_savepoint() --
it returns 'nil, err' in case of an error and returns nothing when
successful. So the check against -1 is not more actual here.
I think we should keep the old box.rollback_to_savepoint() contract as
is at least within the bugfix patch. I mean: raise a Lua error if an
error occurs.
Sorry, I had no enough context and mislead you when proposed to use
luaT_push_nil_and_error(). It is the right way to give an error for new
APIs, but we should keep the old convention for old APIs I think.
> end
> end
>
> diff --git a/src/box/lua/txn.c b/src/box/lua/txn.c
> new file mode 100644
> index 000000000..4362ef403
> --- /dev/null
> +++ b/src/box/lua/txn.c
> @@ -0,0 +1,90 @@
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
2019.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "box/lua/txn.h"
> +#include "box/txn.h"
> +
> +#include <tarantool_ev.h>
> +
> +#include <lua.h>
> +#include <lauxlib.h>
> +#include <lualib.h>
> +
> +#include "lua/utils.h"
> +#include "lua/trigger.h"
> +
> +#include "box/box.h"
> +#include "box/schema.h"
> +
> +struct txn_savepoint*
> +luaT_check_txn_savepoint(struct lua_State *L, int idx)
Should be static.
> +{
> + if (lua_type(L, idx) != LUA_TTABLE)
> + return NULL;
> +
> + lua_getfield(L, idx, "csavepoint");
What if there is no such field?
> +
> + uint32_t cdata_type;
> + struct txn_savepoint **sp_ptr = luaL_checkcdata(L, idx + 1, &cdata_type);
getfield() will add the new item to a top of the stack (idx == 1).
No cdata type check: please, at least describe a problem you met here.
BTW, maybe it is logical to work with cdata in the C module, while
extract it from a table in a Lua part.
> +
> + lua_pop(L, 1);
> +
> + if (sp_ptr == NULL)
> + return NULL;
> +
> + return *sp_ptr;
> +}
> +
> +static int
> +lbox_txn_rollback_to_savepoint(struct lua_State *L)
> +{
> + struct txn_savepoint *point;
> + if (lua_gettop(L) != 1 ||
> + (point = luaT_check_txn_savepoint(L, 1)) == NULL)
> + return luaL_error(L, "Usage: txn:rollback_to_savepoint(savepoint)");
> +
> + int rc = box_txn_rollback_to_savepoint(point);
> + if (rc != 0)
> + return luaT_push_nil_and_error(L);
> +
> + return 0;
> +}
Trailing spaces, mixed tabs / spaces, lines over 80 symbols long.
> +
> +static const struct luaL_Reg lbox_txn_lib[] = {
> + {"rollback_to_savepoint", lbox_txn_rollback_to_savepoint},
> + {NULL, NULL}
> +};
> +
> +void
> +box_lua_txn_init(struct lua_State *L)
> +{
> + luaL_register_module(L, "box.txn", lbox_txn_lib);
> + lua_pop(L, 1);
> +}
> diff --git a/src/box/lua/txn.h b/src/box/lua/txn.h
> new file mode 100644
> index 000000000..299a54886
> --- /dev/null
> +++ b/src/box/lua/txn.h
> @@ -0,0 +1,49 @@
> +#ifndef INCLUDES_TARANTOOL_LUA_TXN_H
> +#define INCLUDES_TARANTOOL_LUA_TXN_H
TARANTOOL_BOX_LUA_TXN_H_INCLUDED, see how other sentry macros are named.
> +
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
2019.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + * copyright notice, this list of conditions and the
> + * following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following
> + * disclaimer in the documentation and/or other materials
> + * provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct lua_State;
> +
> +void
> +box_lua_txn_init(struct lua_State *L);
> +
> +#if defined(__cplusplus)
> +} /* extern "C" */
> +#endif /* defined(__cplusplus) */
> +
> +#endif /* INCLUDES_TARANTOOL_LUA_TXN_H */
> +
The extra empty line at end of the file.
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 38b1b595f..38921a2ff 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -33,6 +33,7 @@
> #include "tuple.h"
> #include "journal.h"
> #include <fiber.h>
> +#include <lauxlib.h>
> #include "xrow.h"
>
> double too_long_threshold;
> @@ -875,3 +876,4 @@ txn_on_yield(struct trigger *trigger, void *event)
> txn_rollback_to_svp(txn, NULL);
> txn_set_flag(txn, TXN_IS_ABORTED_BY_YIELD);
> }
> +
The same: the extra empty line at end of the file.
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread