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

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

[Kirill Shcherbatov]

The _func_index space trigger used to reject an insertion of a
tuple that defines an invalid functional index.
As insertion in _index space had been completed before, a garbage
is kept in _index space in such case.

We need to do something with the yelding _func_index trigger(that
rebuilds an index) to wrap all index creation operation in DDL
transaction (because only the first DDL operation may yeld now).
This problem could be trivially solved with preparatory
initialization of index_def function ponter: the memtx_tree
index construction would perform all required job in such case.
Therefore the following index rebuild in _func_index trigger
becomes redundant and should be omitted.

In other words, a trivial prefetch operation makes possible
a transactional index creation (with space:create_index operation).

As for index construction during recovery (a lack of function
cache during recovery was the main motivation to introduce
_func_index space), it's workflow is kept unchanged.

Follow up #1250
Closes #4401
---
 src/box/index_def.h             | 12 +++++++++
 src/box/alter.cc                | 46 +++++++++++++++++++++++++++------
 src/box/lua/schema.lua          | 40 ++++++++++++++++++----------
 test/engine/func_index.result   | 33 ++++++++++++++++++++---
 test/engine/func_index.test.lua | 14 +++++++---
 5 files changed, 116 insertions(+), 29 deletions(-)

diff --git a/src/box/index_def.h b/src/box/index_def.h
index 62578bd60..d928b23c7 100644
--- a/src/box/index_def.h
+++ b/src/box/index_def.h
@@ -321,6 +321,18 @@ index_def_set_func(struct index_def *def, struct func *func)
 	def->cmp_def->func_index_func = NULL;
 }
 
+/**
+ * Get a func pointer by index definition.
+ * @param def Index def, containing key definitions.
+ * @returns not NULL function pointer when index definition
+ *          refers to function and NULL otherwise.
+ */
+static inline struct func *
+index_def_get_func(struct index_def *def)
+{
+	return def->key_def->func_index_func;
+}
+
 /**
  * Add an index definition to a list, preserving the
  * first position of the primary key.
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
@@ -231,6 +231,23 @@ index_opts_decode(struct index_opts *opts, const char *map,
 	}
 }
 
+/**
+ * Helper routine for functional index function verification:
+ * only a deterministic persistent Lua function may be used in
+ * functional index for now.
+ */
+static void
+func_index_check_func(struct func *func) {
+	assert(func != NULL);
+	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");
+	}
+}
+
 /**
  * Create a index_def object from a record in _index
  * system space.
@@ -285,7 +302,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 				 space->def->fields,
 				 space->def->field_count, &fiber()->gc) != 0)
 		diag_raise();
-	key_def = key_def_new(part_def, part_count, opts.func_id > 0);
+	bool for_func_index = opts.func_id > 0;
+	key_def = key_def_new(part_def, part_count, for_func_index);
 	if (key_def == NULL)
 		diag_raise();
 	struct index_def *index_def =
@@ -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).
+	 */
+	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);
 	} 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()
     end
     return space.index[name]
 end
@@ -1114,6 +1121,8 @@ end
 box.schema.index.drop = function(space_id, index_id)
     check_param(space_id, 'space_id', 'number')
     check_param(index_id, 'index_id', 'number')
+    local _index = box.space[box.schema.INDEX_ID]
+    local _func_index = box.space[box.schema.FUNC_INDEX_ID]
     if index_id == 0 then
         local _space_sequence = box.space[box.schema.SPACE_SEQUENCE_ID]
         local sequence_tuple = _space_sequence:delete{space_id}
@@ -1122,8 +1131,6 @@ box.schema.index.drop = function(space_id, index_id)
             box.schema.sequence.drop(sequence_tuple.sequence_id)
         end
     end
-    local _index = box.space[box.schema.INDEX_ID]
-    local _func_index = box.space[box.schema.FUNC_INDEX_ID]
     for _, v in box.space._func_index:pairs{space_id, index_id} do
         _func_index:delete({v.space_id, v.index_id})
     end
@@ -1229,13 +1236,20 @@ box.schema.index.alter = function(space_id, index_id, options)
     local sequence_proxy = space_sequence_alter_prepare(format, parts, options,
                                                         space_id, index_id,
                                                         space.name, options.name)
-    _index:replace{space_id, index_id, options.name, options.type,
-                   index_opts, parts}
-    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 alter_f = function ()
+        _index:replace{space_id, index_id, options.name, options.type,
+                    index_opts, parts}
+        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
+        space_sequence_alter_commit(sequence_proxy)
+    end
+    if not box.is_in_txn() then
+        box.atomic(alter_f)
+    else
+        alter_f()
     end
-    space_sequence_alter_commit(sequence_proxy)
 end
 
 -- a static box_tuple_t ** instance for calling box_index_* API
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index 877b76d5e..fa0d43b36 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -49,8 +49,13 @@ _ = s:create_index('idx', {func = 6666, parts = {{1, 'unsigned'}}})
  | ---
  | - error: Function '6666' does not exist
  | ...
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
  | ---
+ | - true
+ | ...
+box.space._func_index:count() == 0
+ | ---
+ | - true
  | ...
 -- Can't use non-persistent function in functional index.
 _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'unsigned'}}})
@@ -58,8 +63,13 @@ _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'uns
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
  | ---
+ | - true
+ | ...
+box.space._func_index:count() == 0
+ | ---
+ | - true
  | ...
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
@@ -67,8 +77,13 @@ _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsign
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
  | ---
+ | - true
+ | ...
+box.space._func_index:count() == 0
+ | ---
+ | - true
  | ...
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
@@ -76,8 +91,13 @@ _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsign
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
  | ---
+ | - true
+ | ...
+box.space._func_index:count() == 0
+ | ---
+ | - true
  | ...
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
@@ -731,3 +751,8 @@ box.schema.func.drop('s')
 box.schema.func.drop('sub')
  | ---
  | ...
+
+box.space._func_index:count() == 0
+ | ---
+ | - true
+ | ...
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index 372ec800d..2878c5b73 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -19,16 +19,20 @@ _ = s:create_index('idx', {func = box.func.s.id, parts = {{1, 'unsigned'}}})
 pk = s:create_index('pk')
 -- Invalid fid.
 _ = s:create_index('idx', {func = 6666, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
+box.space._func_index:count() == 0
 -- Can't use non-persistent function in functional index.
 _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
+box.space._func_index:count() == 0
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
+box.space._func_index:count() == 0
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
+box.space._index.index.name:count({s.id, 'idx'}) == 0
+box.space._func_index:count() == 0
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
 -- Can't use parts started not by 1 field.
@@ -248,3 +252,5 @@ idx2:get(3)
 s:drop()
 box.schema.func.drop('s')
 box.schema.func.drop('sub')
+
+box.space._func_index:count() == 0
-- 
2.22.0

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

[Vladimir Davydov]

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.

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

[Kirill Shcherbatov]

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

> 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.
Done. A new test is based on this case.

> 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.
Ok, refactored patch correspondingly.

==============================================

The _func_index space trigger used to reject an insertion of a
tuple that defines an invalid functional index.
As insertion in _index space had been completed before, a garbage
is kept in _index space in such case.

We need to do something with the yelding _func_index trigger(that
rebuilds an index) to wrap all index creation operation in DDL
transaction in further patches (because only the first DDL
operation may yeld now).

This problem could be trivially solved with preparatory
initialization of index_def function ponter: the memtx_tree
index construction would perform all required job in such case.
Therefore the following index rebuild in _func_index trigger
becomes redundant and should be omitted.

In other words, a trivial prefetch operation makes possible
a transactional index creation (with space:create_index operation).

As for index construction during recovery (a lack of function
cache during recovery was the main motivation to introduce
_func_index space), it's workflow is kept unchanged.

Follow up #1250
Needed for #4348
Closes #4401
---
 src/box/index_def.h             | 12 +++++
 src/box/alter.cc                | 58 +++++++++++++++++---
 test/engine/func_index.result   | 96 +++++++++++++++++++++++++++++----
 test/engine/func_index.test.lua | 44 +++++++++++++--
 4 files changed, 191 insertions(+), 19 deletions(-)

diff --git a/src/box/index_def.h b/src/box/index_def.h
index 62578bd60..d928b23c7 100644
--- a/src/box/index_def.h
+++ b/src/box/index_def.h
@@ -321,6 +321,18 @@ index_def_set_func(struct index_def *def, struct func *func)
 	def->cmp_def->func_index_func = NULL;
 }
 
+/**
+ * Get a func pointer by index definition.
+ * @param def Index def, containing key definitions.
+ * @returns not NULL function pointer when index definition
+ *          refers to function and NULL otherwise.
+ */
+static inline struct func *
+index_def_get_func(struct index_def *def)
+{
+	return def->key_def->func_index_func;
+}
+
 /**
  * Add an index definition to a list, preserving the
  * first position of the primary key.
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 92f1d5b22..de778eefd 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -231,6 +231,23 @@ index_opts_decode(struct index_opts *opts, const char *map,
 	}
 }
 
+/**
+ * Helper routine for functional index function verification:
+ * only a deterministic persistent Lua function may be used in
+ * functional index for now.
+ */
+static void
+func_index_check_func(struct func *func) {
+	assert(func != NULL);
+	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");
+	}
+}
+
 /**
  * Create a index_def object from a record in _index
  * system space.
@@ -285,7 +302,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 				 space->def->fields,
 				 space->def->field_count, &fiber()->gc) != 0)
 		diag_raise();
-	key_def = key_def_new(part_def, part_count, opts.func_id > 0);
+	bool for_func_index = opts.func_id > 0;
+	key_def = key_def_new(part_def, part_count, for_func_index);
 	if (key_def == NULL)
 		diag_raise();
 	struct index_def *index_def =
@@ -296,6 +314,26 @@ 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 definition, resolve a
+	 * function pointer to perform a complete index build
+	 * (istead of initializing it in inactive state) in
+	 * on_replace_dd_index trigger. This allows wrap index
+	 * creation operation into transaction: only the first
+	 * opperation in transaction is allowed to yeld.
+	 *
+	 * The initialisation during recovery is slightly
+	 * different, because function cache is not initialized
+	 * during _index space loading. Therefore the completion
+	 * of a functional index creation is performed in
+	 * _func_index space's trigger, via IndexRebuild
+	 * operation.
+	 */
+	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,12 +4763,11 @@ 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);
+		if (index->def->opts.func_id != func->def->fid) {
+			tnt_raise(ClientError, ER_FUNC_INDEX_PARTS,
+				  "functional index function id doesn't match "
+				  "the fid defined in index");
 		}
 	} else if (old_tuple != NULL && new_tuple == NULL) {
 		uint32_t space_id = tuple_field_u32_xc(old_tuple,
@@ -4746,6 +4783,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/test/engine/func_index.result b/test/engine/func_index.result
index 877b76d5e..8bfaf8405 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -58,27 +58,18 @@ _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'uns
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
  | ---
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
  | ---
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
  | ---
@@ -731,3 +722,90 @@ box.schema.func.drop('s')
 box.schema.func.drop('sub')
  | ---
  | ...
+
+--
+-- gh-4401: make functional index creation transactional
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function test1()
+   lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+   box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   box.schema.func.create('extr1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   s = box.schema.space.create('withdata')
+   pk = s:create_index('pk')
+   box.space._index:insert({s.id, 2, 'idx', 'tree', {unique=true, func=box.func.extr.id}, {{0, 'integer'}}})
+   box.space._func_index:insert({s.id, 2, box.func.extr1.id})
+end
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.atomic(test1)
+ | ---
+ | - error: 'Wrong functional index definition: functional index function id doesn''t
+ |     match the fid defined in index'
+ | ...
+
+box.func.extr1 == nil
+ | ---
+ | - true
+ | ...
+box.func.extr == nil
+ | ---
+ | - true
+ | ...
+box.is_in_txn()
+ | ---
+ | - false
+ | ...
+box.space._space.index.name:count('withdata') == 0
+ | ---
+ | - true
+ | ...
+
+-- Test successful index creation
+s = box.schema.space.create('withdata', {engine = engine})
+ | ---
+ | ...
+lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+ | ---
+ | ...
+box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+ | ---
+ | ...
+pk = s:create_index('pk')
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function test2()
+    idx = s:create_index('idx', {unique = true, func = 'extr', parts = {{1, 'integer'}}})
+end
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.atomic(test2)
+ | ---
+ | ...
+
+s:insert({1, 2})
+ | ---
+ | - [1, 2]
+ | ...
+idx:get({3})
+ | ---
+ | - [1, 2]
+ | ...
+
+s:drop()
+ | ---
+ | ...
+box.func.extr:drop()
+ | ---
+ | ...
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index 372ec800d..f31162c97 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -22,13 +22,10 @@ _ = s:create_index('idx', {func = 6666, parts = {{1, 'unsigned'}}})
 s.index.idx:drop()
 -- Can't use non-persistent function in functional index.
 _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
 -- Can't use parts started not by 1 field.
@@ -248,3 +245,44 @@ idx2:get(3)
 s:drop()
 box.schema.func.drop('s')
 box.schema.func.drop('sub')
+
+--
+-- gh-4401: make functional index creation transactional
+--
+test_run:cmd("setopt delimiter ';'")
+function test1()
+   lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+   box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   box.schema.func.create('extr1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   s = box.schema.space.create('withdata')
+   pk = s:create_index('pk')
+   box.space._index:insert({s.id, 2, 'idx', 'tree', {unique=true, func=box.func.extr.id}, {{0, 'integer'}}})
+   box.space._func_index:insert({s.id, 2, box.func.extr1.id})
+end
+test_run:cmd("setopt delimiter ''");
+
+box.atomic(test1)
+
+box.func.extr1 == nil
+box.func.extr == nil
+box.is_in_txn()
+box.space._space.index.name:count('withdata') == 0
+
+-- Test successful index creation
+s = box.schema.space.create('withdata', {engine = engine})
+lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+pk = s:create_index('pk')
+test_run:cmd("setopt delimiter ';'")
+function test2()
+    idx = s:create_index('idx', {unique = true, func = 'extr', parts = {{1, 'integer'}}})
+end
+test_run:cmd("setopt delimiter ''");
+
+box.atomic(test2)
+
+s:insert({1, 2})
+idx:get({3})
+
+s:drop()
+box.func.extr:drop()
-- 
2.22.0

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

[Vladimir Davydov]

The patch is good, just a nit pick below.

On Mon, Aug 05, 2019 at 06:38:51PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 92f1d5b22..de778eefd 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -4725,12 +4763,11 @@ 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);
> +		if (index->def->opts.func_id != func->def->fid) {
> +			tnt_raise(ClientError, ER_FUNC_INDEX_PARTS,
> +				  "functional index function id doesn't match "
> +				  "the fid defined in index");

Why ER_FUNC_INDEX_PARTS? Why not ER_WRONG_INDEX_OPTIONS like in
the error right above? BTW passing 0 to ER_WRONG_INDEX_OPTIONS looks
confusing.

Also, using "function id" and "fid" in the same error message looks ugly.
Please rephrase it. Something like this maybe:

  "Function ids defined in _index and _func_index don't match"

Also, please add a test case covering this particular error.

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

[Kirill Shcherbatov]

On 06.08.2019 16:19, Vladimir Davydov wrote:
> The patch is good, just a nit pick below.

>> +		if (index->def->opts.func_id != func->def->fid) {
>> +			tnt_raise(ClientError, ER_FUNC_INDEX_PARTS,
>> +				  "functional index function id doesn't match "
>> +				  "the fid defined in index");
> 
> Why ER_FUNC_INDEX_PARTS? Why not ER_WRONG_INDEX_OPTIONS like in
> the error right above? BTW passing 0 to ER_WRONG_INDEX_OPTIONS looks
> confusing.
> 
> Also, using "function id" and "fid" in the same error message looks ugly.
> Please rephrase it. Something like this maybe:
> 
>   "Function ids defined in _index and _func_index don't match"
Ok. Got it.

> 
> Also, please add a test case covering this particular error.
> 

Actually it was already there.. =) Upd msg.

=================================================

The _func_index space trigger used to reject an insertion of a
tuple that defines an invalid functional index.
As insertion in _index space had been completed before, a garbage
is kept in _index space in such case.

We need to do something with the yelding _func_index trigger(that
rebuilds an index) to wrap all index creation operation in DDL
transaction in further patches (because only the first DDL
operation may yeld now).

This problem could be trivially solved with preparatory
initialization of index_def function ponter: the memtx_tree
index construction would perform all required job in such case.
Therefore the following index rebuild in _func_index trigger
becomes redundant and should be omitted.

In other words, a trivial prefetch operation makes possible
a transactional index creation (with space:create_index operation).

As for index construction during recovery (a lack of function
cache during recovery was the main motivation to introduce
_func_index space), it's workflow is kept unchanged.

Follow up #1250
Needed for #4348
Closes #4401
---
 src/box/index_def.h             | 12 +++++
 src/box/alter.cc                | 56 ++++++++++++++++---
 test/engine/func_index.result   | 96 +++++++++++++++++++++++++++++----
 test/engine/func_index.test.lua | 44 +++++++++++++--
 4 files changed, 190 insertions(+), 18 deletions(-)

diff --git a/src/box/index_def.h b/src/box/index_def.h
index 62578bd60..d928b23c7 100644
--- a/src/box/index_def.h
+++ b/src/box/index_def.h
@@ -321,6 +321,18 @@ index_def_set_func(struct index_def *def, struct func *func)
 	def->cmp_def->func_index_func = NULL;
 }
 
+/**
+ * Get a func pointer by index definition.
+ * @param def Index def, containing key definitions.
+ * @returns not NULL function pointer when index definition
+ *          refers to function and NULL otherwise.
+ */
+static inline struct func *
+index_def_get_func(struct index_def *def)
+{
+	return def->key_def->func_index_func;
+}
+
 /**
  * Add an index definition to a list, preserving the
  * first position of the primary key.
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 92f1d5b22..aa63488be 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -231,6 +231,23 @@ index_opts_decode(struct index_opts *opts, const char *map,
 	}
 }
 
+/**
+ * Helper routine for functional index function verification:
+ * only a deterministic persistent Lua function may be used in
+ * functional index for now.
+ */
+static void
+func_index_check_func(struct func *func) {
+	assert(func != NULL);
+	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");
+	}
+}
+
 /**
  * Create a index_def object from a record in _index
  * system space.
@@ -285,7 +302,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 				 space->def->fields,
 				 space->def->field_count, &fiber()->gc) != 0)
 		diag_raise();
-	key_def = key_def_new(part_def, part_count, opts.func_id > 0);
+	bool for_func_index = opts.func_id > 0;
+	key_def = key_def_new(part_def, part_count, for_func_index);
 	if (key_def == NULL)
 		diag_raise();
 	struct index_def *index_def =
@@ -296,6 +314,26 @@ 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 definition, resolve a
+	 * function pointer to perform a complete index build
+	 * (istead of initializing it in inactive state) in
+	 * on_replace_dd_index trigger. This allows wrap index
+	 * creation operation into transaction: only the first
+	 * opperation in transaction is allowed to yeld.
+	 *
+	 * The initialisation during recovery is slightly
+	 * different, because function cache is not initialized
+	 * during _index space loading. Therefore the completion
+	 * of a functional index creation is performed in
+	 * _func_index space's trigger, via IndexRebuild
+	 * operation.
+	 */
+	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,12 +4763,11 @@ 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) {
+		func_index_check_func(func);
+		if (index->def->opts.func_id != func->def->fid) {
 			tnt_raise(ClientError, ER_WRONG_INDEX_OPTIONS, 0,
-				  "referenced function doesn't satisfy "
-				  "functional index function constraints");
+				  "Function ids defined in _index and"
+				  "_func_index don't match");
 		}
 	} else if (old_tuple != NULL && new_tuple == NULL) {
 		uint32_t space_id = tuple_field_u32_xc(old_tuple,
@@ -4746,6 +4783,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/test/engine/func_index.result b/test/engine/func_index.result
index 877b76d5e..d7ef9589d 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -58,27 +58,18 @@ _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'uns
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
  | ---
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
  | ---
  | - error: 'Wrong index options (field 0): referenced function doesn''t satisfy functional
  |     index function constraints'
  | ...
-s.index.idx:drop()
- | ---
- | ...
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
  | ---
@@ -731,3 +722,90 @@ box.schema.func.drop('s')
 box.schema.func.drop('sub')
  | ---
  | ...
+
+--
+-- gh-4401: make functional index creation transactional
+--
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function test1()
+   lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+   box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   box.schema.func.create('extr1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   s = box.schema.space.create('withdata')
+   pk = s:create_index('pk')
+   box.space._index:insert({s.id, 2, 'idx', 'tree', {unique=true, func=box.func.extr.id}, {{0, 'integer'}}})
+   box.space._func_index:insert({s.id, 2, box.func.extr1.id})
+end
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.atomic(test1)
+ | ---
+ | - error: 'Wrong index options (field 0): Function ids defined in _index and_func_index
+ |     don''t match'
+ | ...
+
+box.func.extr1 == nil
+ | ---
+ | - true
+ | ...
+box.func.extr == nil
+ | ---
+ | - true
+ | ...
+box.is_in_txn()
+ | ---
+ | - false
+ | ...
+box.space._space.index.name:count('withdata') == 0
+ | ---
+ | - true
+ | ...
+
+-- Test successful index creation
+s = box.schema.space.create('withdata', {engine = engine})
+ | ---
+ | ...
+lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+ | ---
+ | ...
+box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+ | ---
+ | ...
+pk = s:create_index('pk')
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function test2()
+    idx = s:create_index('idx', {unique = true, func = 'extr', parts = {{1, 'integer'}}})
+end
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.atomic(test2)
+ | ---
+ | ...
+
+s:insert({1, 2})
+ | ---
+ | - [1, 2]
+ | ...
+idx:get({3})
+ | ---
+ | - [1, 2]
+ | ...
+
+s:drop()
+ | ---
+ | ...
+box.func.extr:drop()
+ | ---
+ | ...
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index 372ec800d..f31162c97 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -22,13 +22,10 @@ _ = s:create_index('idx', {func = 6666, parts = {{1, 'unsigned'}}})
 s.index.idx:drop()
 -- Can't use non-persistent function in functional index.
 _ = s:create_index('idx', {func = box.func.s_nonpersistent.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-deterministic function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef1.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-sandboxed function in functional index.
 _ = s:create_index('idx', {func = box.func.s_ivaliddef2.id, parts = {{1, 'unsigned'}}})
-s.index.idx:drop()
 -- Can't use non-sequential parts in returned key definition.
 _ = s:create_index('idx', {func = box.func.ss.id, parts = {{1, 'unsigned'}, {3, 'unsigned'}}})
 -- Can't use parts started not by 1 field.
@@ -248,3 +245,44 @@ idx2:get(3)
 s:drop()
 box.schema.func.drop('s')
 box.schema.func.drop('sub')
+
+--
+-- gh-4401: make functional index creation transactional
+--
+test_run:cmd("setopt delimiter ';'")
+function test1()
+   lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+   box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   box.schema.func.create('extr1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+   s = box.schema.space.create('withdata')
+   pk = s:create_index('pk')
+   box.space._index:insert({s.id, 2, 'idx', 'tree', {unique=true, func=box.func.extr.id}, {{0, 'integer'}}})
+   box.space._func_index:insert({s.id, 2, box.func.extr1.id})
+end
+test_run:cmd("setopt delimiter ''");
+
+box.atomic(test1)
+
+box.func.extr1 == nil
+box.func.extr == nil
+box.is_in_txn()
+box.space._space.index.name:count('withdata') == 0
+
+-- Test successful index creation
+s = box.schema.space.create('withdata', {engine = engine})
+lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
+box.schema.func.create('extr', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+pk = s:create_index('pk')
+test_run:cmd("setopt delimiter ';'")
+function test2()
+    idx = s:create_index('idx', {unique = true, func = 'extr', parts = {{1, 'integer'}}})
+end
+test_run:cmd("setopt delimiter ''");
+
+box.atomic(test2)
+
+s:insert({1, 2})
+idx:get({3})
+
+s:drop()
+box.func.extr:drop()
-- 
2.22.0

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

[Vladimir Davydov]

Pushed to master.