From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3169424F80 for ; Mon, 9 Sep 2019 08:07:05 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id sXlliISokR1P for ; Mon, 9 Sep 2019 08:07:05 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5822F24E92 for ; Mon, 9 Sep 2019 08:07:04 -0400 (EDT) Date: Mon, 9 Sep 2019 15:06:46 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] initial version: remove savepoints from FFI Message-ID: <20190909120645.cgmgulz7s2h3ns4u@tkn_work_nb> References: <20190902111414.35583-1-sergos@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190902111414.35583-1-sergos@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: sergos@tarantool.org Cc: tarantool-patches@freelists.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 > > 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 ``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 > + * 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 > + > +#include > +#include > +#include > + > +#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 ``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 > + * 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 > +#include > #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 >