[tarantool-patches] Re: [PATCH] initial version: remove savepoints from FFI

[Alexander Turenko <alexander.turenko@tarantool.org>]

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
>