[tarantool-patches] Re: [PATCH 4/5] Primary index for ephemeral spaces

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 18 comments below.

On 12/07/2018 14:16, imeevma@tarantool.org wrote:
> Functions for creation and deletion primary index
> of ephemeral space added.
> 
> Part of #3375.
> ---
>   src/box/index_def.c               |  42 +++++++
>   src/box/index_def.h               |  18 +++
>   src/box/lua/schema.lua            | 130 ++++++++++++++++++++++
>   src/box/lua/space.cc              |  92 ++++++++++++++++
>   test/box/ephemeral_space.result   | 225 ++++++++++++++++++++++++++++++++++++--
>   test/box/ephemeral_space.test.lua |  88 +++++++++++++--
>   test/engine/iterator.result       |   2 +-
>   7 files changed, 582 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/index_def.c b/src/box/index_def.c
> index e0d54c0..9622dce 100644
> --- a/src/box/index_def.c
> +++ b/src/box/index_def.c
> @@ -31,6 +31,8 @@
>   #include "index_def.h"
>   #include "schema_def.h"
>   #include "identifier.h"
> +#include "fiber.h"
> +#include "space.h"

1. Please, do not make index_def depend on space.
Instead of passing struct space to ephemeral_index_def use
directly field_def * array and field_count parameters.

2. Can you find a way how to do not duplicate this code
from alter.cc index_def_new_from_tuple?

>   
>   const char *index_type_strs[] = { "HASH", "TREE", "BITSET", "RTREE" };
>   
> @@ -369,3 +371,43 @@ index_opts_decode(struct index_opts *opts, const char *map,
>   	}
>   	return 0;
>   }
> +
> +struct index_def *
> +ephemeral_index_def(struct space *space, uint32_t index_id,

3. Respect the code style:

_new, _delete names for allocating, initialization and deletion.

> +		    const char *name, const char *type_field,
> +		    const char *opts_field, const char *parts)
> +{
> +	const struct field_def *fields = space->def->fields;
> +	uint32_t field_count = space->def->field_count;
> +	struct index_opts opts;
> +	struct key_def *key_def = NULL;
> +	uint32_t part_count = mp_decode_array(&parts);
> +	enum index_type type = STR2ENUM(index_type, type_field);
> +	if(index_opts_decode(&opts, opts_field, &fiber()->gc) != 0)

4. + white space after 'if'. I will not mention it anymore, but please,
find other places yourself.

> +		return NULL;
> +	if(identifier_check(name, strlen(name)) != 0)
> +		return NULL;
> +	struct key_part_def *part_def = (struct key_part_def *)
> +			malloc(sizeof(*part_def) * part_count);

5. Wrong alignment. Rather this:

	struct key_part_def *part_def =
		(struct key_part_def *) malloc(sizeof(*part_def) * part_count);

6. You can use region here and do not free it now, but later using
region_truncate. It reduces count of malloc-free oscillation.

> +	if (part_def == NULL) {
> +		diag_set(OutOfMemory, sizeof(*part_def) * part_count,
> +			 "malloc", "key_part_def");
> +		return NULL;
> +	}
> +	if (key_def_decode_parts(part_def, part_count, &parts,
> +				 fields, field_count) != 0) {
> +		free(part_def);
> +		return NULL;
> +	}
> +	key_def = key_def_new_with_parts(part_def, part_count);
> +	free(part_def);
> +	if (key_def == NULL)
> +		return NULL;
> +	struct index_def *index_def =
> +		index_def_new(0, index_id, name, strlen(name),
> +			      type, &opts, key_def, NULL);
> +	key_def_delete(key_def);
> +	if (index_def == NULL)
> +		return NULL;
> +	return index_def;

7. These 3 lines can be replaced with one 'return index_def'.

> +}
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 81f0fa0..0fe109a 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -527,6 +527,9 @@ box.schema.space.drop_ephemeral = function(ephemeral_space)
>           return
>       end
>       check_param(ephemeral_space.space, 'space', 'cdata')
> +    if ephemeral_space.index ~= nil and ephemeral_space.index[0] ~= nil then

8. How ephemeral_space.index can be nil after the check above (about
ephemeral_space.space == nil)?

> +        ephemeral_space.index[0]:drop()
> +    end
>       space_delete_ephemeral(ephemeral_space)
>       for k,_ in pairs(ephemeral_space) do
>           ephemeral_space[k] = nil
> @@ -1084,6 +1087,128 @@ ffi.metatype(iterator_t, {
>       end;
>   })
>   
> +local index_new_ephemeral = box.internal.space.index_new_ephemeral
> +box.internal.space.index_new_ephemeral = nil
> +local index_delete_ephemeral = box.internal.space.index_delete_ephemeral
> +box.internal.space.index_delete_ephemeral = nil
> +local index_ephemeral_methods = box.schema.index_ephemeral_methods
> +box.schema.index_ephemeral_mt = nil
> +local index_ephemeral_mt = {}
> +
> +local index_ephemeral_create = function(ephemeral_space, name, options)

9. Lets do the same API as for non-ephemeral spaces:
create_index, not index_create.

> +    if type(ephemeral_space) ~= 'table' then
> +        error("Usage: ephemeral_space:create_index(name, opts)")
> +    end
> +    check_param(name, 'name', 'string')
> +    check_param_table(options, create_index_template)
> +    local space = ephemeral_space
> +    local format = space:format()
> +
> +    local options_defaults = {
> +        type = 'tree',
> +    }
> +    options = update_param_table(options, options_defaults)
> +    local type_dependent_defaults = {
> +        rtree = {parts = { 2, 'array' }, unique = false},
> +        bitset = {parts = { 2, 'unsigned' }, unique = false},
> +        other = {parts = { 1, 'unsigned' }, unique = true},
> +    }
> +    options_defaults = type_dependent_defaults[options.type]
> +            or type_dependent_defaults.other
> +    if not options.parts then
> +        local fieldno = options_defaults.parts[1]
> +        if #format >= fieldno then
> +            local t = format[fieldno].type
> +            if t ~= 'any' then
> +                options.parts = {{fieldno, format[fieldno].type}}
> +            end
> +        end
> +    end
> +    options = update_param_table(options, options_defaults)
> +    if space.engine == 'vinyl' then> +        options_defaults = {
> +            page_size = box.cfg.vinyl_page_size,
> +            range_size = box.cfg.vinyl_range_size,
> +            run_count_per_level = box.cfg.vinyl_run_count_per_level,
> +            run_size_ratio = box.cfg.vinyl_run_size_ratio,
> +            bloom_fpr = box.cfg.vinyl_bloom_fpr
> +        }
> +    else
> +        options_defaults = {}
> +    end
> +    options = update_param_table(options, options_defaults)
> +
> +    if ephemeral_space.index ~= nil and ephemeral_space.index[0] then
> +        if options.if_not_exists then
> +            return space.index[0], "not created"
> +        else
> +            error("Primary index for this ephemeral index already exists")
> +        end
> +    end
> +    local iid = 0
> +    if options.id and options.id ~= 0 then
> +        error("Ephemeral space can have only primary index.")
> +    end
> +    local parts, parts_can_be_simplified =
> +        update_index_parts(format, options.parts)
> +    -- create_index() options contains type, parts, etc,
> +    -- stored separately. Remove these members from index_opts
> +    local index_opts = {
> +            dimension = options.dimension,
> +            unique = options.unique,
> +            distance = options.distance,
> +            page_size = options.page_size,
> +            range_size = options.range_size,
> +            run_count_per_level = options.run_count_per_level,
> +            run_size_ratio = options.run_size_ratio,
> +            bloom_fpr = options.bloom_fpr,
> +    }
> +    local field_type_aliases = {
> +        num = 'unsigned'; -- Deprecated since 1.7.2
> +        uint = 'unsigned';
> +        str = 'string';
> +        int = 'integer';
> +        ['*'] = 'any';
> +    };
> +    for _, part in pairs(parts) do
> +        local field_type = part.type:lower()
> +        part.type = field_type_aliases[field_type] or field_type
> +        if field_type == 'num' then
> +            log.warn("field type '%s' is deprecated since Tarantool 1.7, "..
> +                     "please use '%s' instead", field_type, part.type)
> +        end
> +    end
> +    if parts_can_be_simplified then
> +        parts = simplify_index_parts(parts)
> +    end

10. This function again is almost complete duplicate of schema.index.create.
Please, remove code duplication.

> +    space.space = index_new_ephemeral(space, iid, name, options.type,
> +                                      msgpack.encode(index_opts),
> +                                      msgpack.encode(parts))
> +    space.index[iid] = {}
> +    space.index[iid].unique = index_opts.unique or true
> +    space.index[iid].parts = parts
> +    space.index[iid].id = iid
> +    space.index[iid].name = name
> +    space.index[iid].type = type
> +    space.index[iid].options = options
> +    space.index[iid].space = space.space
> +    space.index[iid].ephemeral_space = space
> +    space.index[name] = space.index[iid]
> +    setmetatable(space.index[iid], index_ephemeral_mt)
> +    return space.index[name]
> +end
> +
> +local index_ephemeral_drop = function(ephemeral_index)
> +    if type(ephemeral_index) ~= 'table' then
> +        error("Usage: ephemeral_space.index[0]:drop()")
> +    end
> +    ephemeral_index.ephemeral_space.space = index_delete_ephemeral(ephemeral_index)
> +    ephemeral_index.ephemeral_space.index = {}
> +    for k,_ in pairs(ephemeral_index) do
> +        ephemeral_index[k] = nil
> +    end

11. It will not work, when we will allow multiple indexes. You should not
delete and nullify all indexes when a one is deleted.

> +end
> +
>   local iterator_gen = function(param, state)
>       --[[
>           index:pairs() mostly conforms to the Lua for-in loop conventions and
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 955c465..4374db9 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -34,6 +34,7 @@
>   #include "lua/utils.h"
>   #include "lua/trigger.h"
>   #include "box/box.h"
> +#include "box/lua/misc.h" /* lbox_encode_tuple_on_gc() */

12. Why? Please, go through your patches and check for unused
includes. I will not mention it anymore.

>   
>   extern "C" {
>   	#include <lua.h>
> @@ -616,6 +617,93 @@ lbox_space_delete_ephemeral(struct lua_State *L)
>   }
>   
>   /**
> + * Create an index for ephemeral space.
> + * @param Lua table - ephemeral space.
> + * @param Lua uint32_t id - id of index, for
> + * ephemeral spaces it is 0 as they can have only
> + * primary index.
> + * @param Lua const char *name - name of index.
> + * @param Lua const char *type_field - type of field.
> + * @param Lua const char *opts_field - options in
> + * msgpack format.
> + * @param Lua const char *parts - parts in msgpack
> + * format.
> + * @retval not nil - ephemeral space created.
> + * @retval nil - error, A reason is returned in
> + *         the second value.
> + */
> +static int
> +lbox_index_new_ephemeral(struct lua_State *L)
> +{
> +	uint32_t argc = lua_gettop(L);
> +	if (argc != 6)
> +		return luaL_error(L, "Using: ephemeral:create_index(opts");
> +	struct space *space = (struct space *)lua_checkephemeralspace(L, 1);
> +	uint32_t index_id = luaL_checknumber (L, 2);
> +	const char *name = luaL_checkstring (L, 3);
> +	const char *type_field = luaL_checkstring (L, 4);
> +	const char *opts_field = luaL_checkstring (L, 5);
> +	const char *parts = luaL_checkstring (L, 6);
> +	assert(index_id == 0);
> +	struct space_def *ephemeral_space_def = space_def_dup(space->def);

13. Please, remove '_ephemeral' suffix from variable names. They are too
long and it is hard to read this Java/C#-like code.

> +	if (ephemeral_space_def == NULL)
> +		return luaL_error(L, "Error with creating space_def");

14. Same as earlier - do not override Tarantool error with Lua one.
Use luaT_error. I will not mention it anymore, so, please, found and
fix other places.

> +	struct index_def *index_def_ephemeral =
> +		ephemeral_index_def(space, index_id, name, type_field,
> +				    opts_field, parts);
> +	if(index_def_ephemeral == NULL)
> +		return luaT_error(L);
> +	if(!index_def_is_valid(index_def_ephemeral, space_name(space)) ||
> +	   space_check_index_def(space, index_def_ephemeral) != 0) {
> +		index_def_delete(index_def_ephemeral);
> +		return luaT_error(L);
> +	}
> +	struct rlist key_list;
> +	rlist_create(&key_list);
> +	rlist_add_entry(&key_list, index_def_ephemeral, link);
> +	struct space *new_space = space_new_ephemeral(ephemeral_space_def,
> +						      &key_list);
> +	index_def_delete(index_def_ephemeral);
> +	space_def_delete(ephemeral_space_def);
> +	if(new_space == NULL) {
> +		return luaL_error(L, "Error with creating index for "\
> +				     "ephemeral space");
> +	}
> +	space_delete(space);
> +	struct space **ptr =
> +		(struct space **) luaL_pushcdata(L, CTID_STRUCT_SPACE_POINTER);
> +	*ptr = new_space;
> +	return 1;
> +}
> diff --git a/test/box/ephemeral_space.result b/test/box/ephemeral_space.result
> index 135df3c..d958ffe 100644
> --- a/test/box/ephemeral_space.result
> +++ b/test/box/ephemeral_space.result
> @@ -1,5 +1,5 @@
> --- Ephemeral space: creation and dropping
> --- Simple creation
> +-- Ephemeral space: creation and dropping.
> +-- Simple creation.

15. These spelling fixes should not be part of this patch.

>   s = box.schema.space.create_ephemeral()
>   ---
>   ...
> @@ -185,7 +185,218 @@ s
>   ---
>   - []
>   ...
> --- Ephemeral space: methods
> +-- Ephemeral space: index creation and dropping.
> +s = box.schema.space.create_ephemeral()
> +---
> +...
> +-- Simple creation
> +i = s:create_index('a')
> +---
> +...
> +i.unique
> +---
> +- true
> +...
> +i.parts
> +---
> +- - - 0
> +    - unsigned
> +...
> +i.id
> +---
> +- 0
> +...
> +i.name
> +---
> +- a
> +...
> +i:drop()
> +---
> +...
> +-- Double creation of index for ephemeral space.
> +i = s:create_index('a')
> +---
> +...
> +i = s:create_index('a')
> +---
> +- error: 'builtin/box/schema.lua:1145: Primary index for this ephemeral index already
> +    exists'

16. Please, use diag_set and ER_UNSUPPORTED for this type of errors.

> +...
> +i:drop()
> +---
> +...
> +-- Double creation of index for ephemeral space
> +-- but with if_not_exists=true.
> +i = s:create_index('a')
> +---
> +...
> +i = s:create_index('a', {if_not_exists=true})
> +---
> +...
> +i:drop()
> +---
> +...
> +-- Ephemeral space can have only primary.
> +-- index so its id == 0
> +i = s:create_index('a', {id = 10})
> +---
> +- error: 'builtin/box/schema.lua:1150: Ephemeral space can have only primary index.'
> +...
> +-- Set parameters: parts.
> +i = s:create_index('a', {parts={1}})
> +---
> +...
> +i.unique
> +---
> +- true

17. It is enough to just print the entire 'i'. Just write it alone on
the line. And it is not necessary to output the entire 'i' on each
'create_index'. Print just the attributes you have specified in the
options.

> +---
> +...
> +i = s:create_index('a', {parts={1,'uint',2,'int',3,'str'}})

18. Please, do not use 'uint' and 'str' - they are deprecated.