[tarantool-patches] Re: [PATCH 3/5] Ephemeral space creation and deletion in Lua

[Vladislav Shpilevoy <v.shpilevoy@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 <lua.h>
> @@ -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.