From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 5 Aug 2019 13:20:45 +0300 From: Vladimir Davydov Subject: Re: [PATCH v1 1/1] box: make functional index creation transactional Message-ID: <20190805102045.GA4242@esperanza> References: <1056e3dd6be0f9d03c6feff3651d41884cbd91c6.1564751087.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1056e3dd6be0f9d03c6feff3651d41884cbd91c6.1564751087.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: 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.