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 AC08A273F0 for ; Fri, 13 Jul 2018 12:32:16 -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 fyI4QzKW8rie for ; Fri, 13 Jul 2018 12:32:16 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 40FAB2744D for ; Fri, 13 Jul 2018 12:32:16 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/5] Ephemeral space creation and deletion in Lua References: <877bbdb510d00753bc61ebbd17bb501a5948292f.1531393821.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <9d90b258-465e-f6cd-1a23-23bf909c8b95@tarantool.org> Date: Fri, 13 Jul 2018 19:32:14 +0300 MIME-Version: 1.0 In-Reply-To: <877bbdb510d00753bc61ebbd17bb501a5948292f.1531393821.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, imeevma@tarantool.org Thanks for the patch! See 18 comments below. On 12/07/2018 14:16, imeevma@tarantool.org wrote: > Import functions to create ephemeral space in Lua and some > its methods that do not require index. > > Part of #3375. > --- > src/box/lua/schema.lua | 69 ++++++++++ > src/box/lua/space.cc | 183 +++++++++++++++++++++++++ > test/box/ephemeral_space.result | 271 ++++++++++++++++++++++++++++++++++++++ > test/box/ephemeral_space.test.lua | 96 ++++++++++++++ > test/engine/iterator.result | 2 +- > 5 files changed, 620 insertions(+), 1 deletion(-) > create mode 100644 test/box/ephemeral_space.result > create mode 100644 test/box/ephemeral_space.test.lua > > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index e4c1b1d..81f0fa0 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -477,6 +477,64 @@ box.schema.space.create = function(name, options) > return box.space[id], "created" > end > > +local space_new_ephemeral = box.internal.space.space_new_ephemeral > +box.internal.space.space_new_ephemeral = nil > +local space_delete_ephemeral = box.internal.space.space_delete_ephemeral > +box.internal.space.space_delete_ephemeral = nil > +local space_ephemeral_methods = box.schema.space_ephemeral_methods > +box.schema.space_ephemeral_methods = nil > +local space_ephemeral_mt = {} > + > +box.schema.space.create_ephemeral = function(options) > + local options_template = { > + engine = 'string', > + field_count = 'number', > + format = 'table', > + } > + local options_defaults = { > + engine = 'memtx', > + field_count = 0, > + } > + check_param_table(options, options_template) > + options = update_param_table(options, options_defaults) > + > + local format = options.format and options.format or {} > + check_param(format, 'format', 'table') > + format = update_format(format) > + local packed_format = msgpack.encode(format) 1. You can pass format as a Lua table into space_new_ephemeral and encode it inside. It is strange to pass binary data into Lua function, that does not work with network or disk. > + > + local ephemeral_space = {} > + ephemeral_space.space = space_new_ephemeral(options.engine, > + options.field_count, > + packed_format) > + ephemeral_space.space_format = format > + ephemeral_space.engine = options.engine > + ephemeral_space.field_count = options.field_count > + ephemeral_space.temporary = true 2. I think, the space should not have this flag at all. Neither true nor false. > + ephemeral_space.index = {} > + -- Set GC for result > + setmetatable(ephemeral_space, space_ephemeral_mt) > + ephemeral_space.proxy = newproxy(true) > + getmetatable(ephemeral_space.proxy).__gc = function(self) > + box.schema.space.drop_ephemeral(ephemeral_space) 3. Why not 'ephemeral_space:drop()' ? > + end > + -- Return result 4. Unhelpful comment. > + return ephemeral_space > +end > + > +box.schema.space.drop_ephemeral = function(ephemeral_space) > + if ephemeral_space == nil or ephemeral_space.space == nil then > + return 5. How is it possible, that ephemeral_space == nil? > + end > + check_param(ephemeral_space.space, 'space', 'cdata') > + space_delete_ephemeral(ephemeral_space) 6. It is strange, that space_new_ephemeral returns space *, but space_delete_ephemeral takes Lua table. Please, make space_delete_ephemeral taking space * as well. > + for k,_ in pairs(ephemeral_space) do > + ephemeral_space[k] = nil > + end 7. After drop any DML/DQL method should throw an error that the space is deleted. You can do it via a separate metatable for example. For example, like this: dropped_mt = { __index = function() error('The space is dropped and can not be used') end } setmetatable(space, dropped_mt) > diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc > index ca3fefc..955c465 100644 > --- a/src/box/lua/space.cc > +++ b/src/box/lua/space.cc > @@ -33,6 +33,7 @@ > #include "box/sql/sqliteLimit.h" > #include "lua/utils.h" > #include "lua/trigger.h" > +#include "box/box.h" 8. Why? > > extern "C" { > #include > @@ -508,6 +511,169 @@ usage_error: > return luaL_error(L, "Usage: space:frommap(map, opts)"); > } > > +/** > + * Check if given argument have an appropriate type. > + * @param Lua struct space *space - ephemeral space. > + * @retval struct space *space - success. > + * @retval NULL - error. > + */ > +static inline struct space * > +luaT_isspace(struct lua_State *L, int narg) 9. 'narg' is rather about argument count. Lets better use 'idx' like in Lua sources. Same about functions below. 10. Kostja asked to describe parameters for Lua C function in the same way as for internal C functions. So lets describe it somehow like this: @param L Lua stack to get space from. @param idx Index by which to get the space from @a L. Same about comments of other functions. > +{ > + uint32_t ctypeid = 0; > + void *data; 11. You do not need to announce 'void *data' before its assignment. > + > + if (lua_type(L, narg) != LUA_TCDATA) > + return NULL; > + > + data = luaL_checkcdata(L, narg, &ctypeid); > + if (ctypeid != CTID_STRUCT_SPACE_POINTER) > + return NULL; > + > + return *(struct space **) data; > +} > + > +/** > + * Puts space field of given argument in stack. 12. It puts, but why is it mentioned in the comment, if you pop the space just after checking? It actually just checks with no after-effects. > + * onto Lua state stack and calls luaT_isspace > + * @param Lua table with field space. > + * @retval struct space *space - success. > + * @retval NULL - error. 13. It does not return NULL. > + */ > +static inline struct space * > +lua_checkephemeralspace(struct lua_State *L, int narg) 14. Why do you need 'narg' here, if it is ignored in getfield and isspace, and used for error message only? > +{ > + lua_getfield(L, 1, "space"); > + struct space *space = luaT_isspace(L, -1); > + lua_pop(L, 1); > + if (space == NULL) { > + luaL_error(L, "Invalid argument #%d (ephemeral space expected,"\ > + "got %s)", narg, lua_typename(L, lua_type(L, narg))); > + } > + return space; > +} > + > +/** > + * Create an ephemeral space. > + * @param Lua const char *engine_name - name of engine. > + * @param Lua uint32_t field_count - number of fields. > + * @param Lua const char *format - format in msgpack. > + * @retval not nil - ephemeral space created. > + * @retval nil - error, A reason is returned in > + * the second value. > + */ > +static int > +lbox_space_new_ephemeral(struct lua_State *L) > +{ > + uint32_t argc = lua_gettop(L); > + if (argc != 3) > + return luaL_error(L, "Error with creating ephemeral space"); > + const char *engine_name = luaL_checkstring (L, 1); > + uint32_t exact_field_count = luaL_checknumber (L, 2); > + const char *format = luaL_checkstring (L, 3); > + struct region *region = &fiber()->gc; > + uint32_t field_count; > + struct field_def *fields = space_format_decode(format, &field_count, > + "ephemeral", strlen("ephemeral"), ER_CREATE_SPACE, region); 15. You do not check space_format_decode on error. > + if (exact_field_count != 0 && > + exact_field_count < field_count) { > + return luaL_error(L, "exact_field_count must be either 0 or"\ > + ">= formatted field count"); > + } > + struct space_def *ephemeral_space_def = > + space_def_new(0, 0, exact_field_count, "ephemeral", > + strlen("ephemeral"), engine_name, > + strlen(engine_name), &space_opts_default, fields, > + field_count); > + if (ephemeral_space_def == NULL) > + return luaL_error(L, "Error with creating space_def"); 16. Not luaL_error. You should rethrow the tarantool error to Lua. See luaT_error. Same about errors below. > + struct rlist key_list; > + rlist_create(&key_list); > + struct space *space = space_new_ephemeral(ephemeral_space_def, > + &key_list); > + space_def_delete(ephemeral_space_def); > + if (space == NULL) > + return luaL_error(L, "Error with creating space"); > + struct space **ptr = > + (struct space **) luaL_pushcdata(L, CTID_STRUCT_SPACE_POINTER); > + *ptr = space; > + return 1; 17. Space_format_decode produces garbage on region, that you should clean. > + > +/** > + * Make a tuple or a table Lua object by map. > + * @param Lua space object. > + * @param Lua map table object. > + * @param Lua opts table object (optional). > + * @retval not nil A tuple or a table conforming to a space > + * format. > + * @retval nil, err Can not built a tuple. A reason is returned in > + * the second value. > + */ > +static int > +lbox_space_frommap_ephemeral(struct lua_State *L) 18. This function is almost completely the same as lbox_space_frommap. Please, remove code duplication.