Re: [PATCH v1 1/1] box: make functional index creation transactional

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Fri, Aug 02, 2019 at 04:05:06PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 92f1d5b22..b92ed056f 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -296,6 +314,16 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
>  	auto index_def_guard = make_scoped_guard([=] { index_def_delete(index_def); });
>  	index_def_check_xc(index_def, space_name(space));
>  	space_check_index_def_xc(space, index_def);
> +	/*
> +	 * In case of functional index key definition,
> +	 * initialize a function pointer, when possible
> +	 * (but not during recovery).
> +	 */

Please improve the comment so that it's clear what problem we solve
here.

> +	struct func *func = NULL;
> +	if (for_func_index && (func = func_by_id(opts.func_id)) != NULL) {
> +		func_index_check_func(func);
> +		index_def_set_func(index_def, func);
> +	}
>  	if (index_def->iid == 0 && space->sequence != NULL)
>  		index_def_check_sequence(index_def, space->sequence_fieldno,
>  					 space->sequence_path,
> @@ -4725,13 +4753,8 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
>  		space = space_cache_find_xc(space_id);
>  		index = index_find_xc(space, index_id);
>  		func = func_cache_find(fid);
> -		if (func->def->language != FUNC_LANGUAGE_LUA ||
> -		    func->def->body == NULL || !func->def->is_deterministic ||
> -		    !func->def->is_sandboxed) {
> -			tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, 0,
> -				  "referenced function doesn't satisfy "
> -				  "functional index function constraints");
> -		}
> +		func_index_check_func(func);
> +		assert(index->def->opts.func_id == func->def->fid);

Technically, an assertion isn't quite correct here, because one may
insert tuples into _index and _func_index that have mismatching function
ids. Please replace it with an error and make sure it's covered by a
test.

>  	} else if (old_tuple != NULL && new_tuple == NULL) {
>  		uint32_t space_id = tuple_field_u32_xc(old_tuple,
>  					BOX_FUNC_INDEX_FIELD_SPACE_ID);
> @@ -4746,6 +4769,13 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
>  			  "functional index", "alter");
>  	}
>  
> +	/**
> +	 * Index is already initialized for corresponding
> +	 * function. Index rebuild is not required.
> +	 */
> +	if (index_def_get_func(index->def) == func)
> +		return;
> +
>  	alter = alter_space_new(space);
>  	auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);});
>  	alter_space_move_indexes(alter, 0, index->def->iid);
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 65017d391..89a4d25bc 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1102,11 +1102,18 @@ box.schema.index.create = function(space_id, name, options)
>      local sequence_proxy = space_sequence_alter_prepare(format, parts, options,
>                                                          space_id, iid,
>                                                          space.name, name)
> -    _index:insert{space_id, iid, name, options.type, index_opts, parts}
> -    space_sequence_alter_commit(sequence_proxy)
> -    if index_opts.func ~= nil then
> -        local _func_index = box.space[box.schema.FUNC_INDEX_ID]
> -        _func_index:insert{space_id, iid, index_opts.func}
> +    local create_f = function ()
> +        _index:insert{space_id, iid, name, options.type, index_opts, parts}
> +        space_sequence_alter_commit(sequence_proxy)
> +        if index_opts.func ~= nil then
> +            local _func_index = box.space[box.schema.FUNC_INDEX_ID]
> +            _func_index:insert{space_id, iid, index_opts.func}
> +        end
> +    end
> +    if not box.is_in_txn() then
> +        box.atomic(create_f)
> +    else
> +        create_f()

All you need to do in the scope of this issue is allow to call
index.create from a user transaction. There's a separate issue for
making all Lua DDL operations transactional (see #4348) and it should be
fixed in a unified manner for all DDL-related functions (with the aid of
a function decorator that would add box.begin/commit if necessary, I
guess).

Please don't patch index.create/alter in this patch. The tests should
just check that those functions work fine in a user transaction. The
rest should be done in a separate patch in the scope of #4348.