From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 10 Jun 2019 15:10:52 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 5/9] schema: rework _func system space format Message-ID: <20190610121052.wyoncz3l53pg5ngl@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 06, 2019 at 03:04:01PM +0300, Kirill Shcherbatov wrote: > This patch updates a _func system space to prepare it for > working with persistent functions. The format of the _func > system space is > [ UINT, UINT, STR, UINT, > STR, STR, STR, > BOOL, MAP] > > It is preparational step to introduce persistent Lua function > in Tarantool. > > The new field is a string that represents the function > body, returns is the type of value that it returns, and the > is_deterministic flag is responsible whether this routine > deterministic or not (can produce only one result for a given > list of parameters). > > Updated function definition structure, decode operation and > migration script correspondingly. > > Part of #4182 > Needed for #1260 > --- > src/box/alter.cc | 61 ++++++++++++++++++++++++++++------- > src/box/bootstrap.snap | Bin 4393 -> 4449 bytes > src/box/func_def.h | 14 ++++++-- > src/box/lua/schema.lua | 13 ++++++-- > src/box/lua/upgrade.lua | 25 +++++++++++++- > src/box/schema_def.h | 4 +++ > test/box-py/bootstrap.result | 6 ++-- > test/box/access_misc.result | 6 ++-- > 8 files changed, 108 insertions(+), 21 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 3b57a7d82..11cad77c3 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -2426,26 +2426,45 @@ func_def_get_ids_from_tuple(struct tuple *tuple, uint32_t *fid, uint32_t *uid) > static struct func_def * > func_def_new_from_tuple(struct tuple *tuple) > { > - uint32_t len; > - const char *name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME, > - &len); > - if (len > BOX_NAME_MAX) > + uint32_t field_count = tuple_field_count(tuple); > + uint32_t name_len, body_len; > + const char *name, *body; > + name = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_NAME, &name_len); > + if (name_len > BOX_NAME_MAX) { > tnt_raise(ClientError, ER_CREATE_FUNCTION, > tt_cstr(name, BOX_INVALID_NAME_MAX), > "function name is too long"); > - identifier_check_xc(name, len); > - struct func_def *def = (struct func_def *) malloc(func_def_sizeof(len)); > + } > + identifier_check_xc(name, name_len); > + if (field_count > BOX_FUNC_FIELD_BODY) { > + body = tuple_field_str_xc(tuple, BOX_FUNC_FIELD_BODY, > + &body_len); > + } else { > + body = NULL; > + body_len = 0; > + } > + > + uint32_t def_sz = func_def_sizeof(name_len, body_len); > + struct func_def *def = (struct func_def *) malloc(def_sz); > if (def == NULL) > - tnt_raise(OutOfMemory, func_def_sizeof(len), "malloc", "def"); > + tnt_raise(OutOfMemory, def_sz, "malloc", "def"); > auto def_guard = make_scoped_guard([=] { free(def); }); > func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid); > - memcpy(def->name, name, len); > - def->name[len] = 0; > - if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID) > + memcpy(def->name, name, name_len); > + def->name[name_len] = 0; > + if (body_len > 0) { > + def->body = def->name + name_len + 1; > + memcpy(def->body, body, body_len); > + def->body[body_len] = 0; > + } else { > + def->body = NULL; > + } > + > + if (field_count > BOX_FUNC_FIELD_SETUID) > def->setuid = tuple_field_u32_xc(tuple, BOX_FUNC_FIELD_SETUID); > else > def->setuid = false; > - if (tuple_field_count(tuple) > BOX_FUNC_FIELD_LANGUAGE) { > + if (field_count > BOX_FUNC_FIELD_LANGUAGE) { > const char *language = > tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_LANGUAGE); > def->language = STR2ENUM(func_language, language); > @@ -2457,6 +2476,26 @@ func_def_new_from_tuple(struct tuple *tuple) > /* Lua is the default. */ > def->language = FUNC_LANGUAGE_LUA; > } > + if (def->language != FUNC_LANGUAGE_LUA && body_len > 0) { > + tnt_raise(ClientError, ER_CREATE_FUNCTION, name, > + "function body may be specified only for " > + "Lua language"); > + } This check should live in the specific func constructor so that we don't need to touch it when we introduce SQL functions. > + if (field_count > BOX_FUNC_FIELD_BODY) { > + assert(field_count > BOX_FUNC_FIELD_OPTS); > + const char *type = > + tuple_field_cstr_xc(tuple, BOX_FUNC_FIELD_RETURNS); > + def->returns = STR2ENUM(field_type, type); > + if (def->returns == field_type_MAX) { > + tnt_raise(ClientError, ER_CREATE_FUNCTION, name, > + "function return value has unknown field type"); > + } > + def->is_deterministic = > + tuple_field_bool_xc(tuple, BOX_FUNC_FIELD_IS_DETERMINISTIC); > + } else { > + def->returns = FIELD_TYPE_ANY; > + def->is_deterministic = false; > + } > def_guard.is_active = false; > return def; > } > diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap > index bb8fbeba114b1e72a5585548fb7f22796931d90f..440495684401541854e9d9180f55cc9a4bc45b25 100644 > > diff --git a/src/box/func_def.h b/src/box/func_def.h > index 5b52ab498..7a920f65e 100644 > --- a/src/box/func_def.h > +++ b/src/box/func_def.h > @@ -32,6 +32,7 @@ > */ > > #include "trivia/util.h" > +#include "field_def.h" > #include > > /** > @@ -54,11 +55,17 @@ struct func_def { > uint32_t fid; > /** Owner of the function. */ > uint32_t uid; > + /** Definition of the persistent function. */ > + char *body; > /** > * True if the function requires change of user id before > * invocation. > */ > bool setuid; > + /** The type of the returned value. */ > + enum field_type returns; As discussed verbally, the only reason we need 'returns' now is to differentiate between multikey and unikey functional indexes and so it can be either 'array' or something else we don't care what exactly. We don't check function return value against this field. This doesn't look right to me. IMO creating a multikey index judging by a function definition looks indirect and confusing - after all whether an index is multikey or not is a property of the index itself or, to be more exact, of the relationship between the index and the function. That being said, I think we better drop 'returns' property for now and instead allow to specify is_multikey flag when creating a functional index, like this: box.space.my_space:create_index('my_index', {function = {id = 'my_func', is_multikey = true}}) Let's add 'returns' (and possibly 'args') when we really need to check function return value. > + /** Whether this function is deterministic. */ > + bool is_deterministic; Please try to keep bools together unless it hurts readability as this reduces the struct size.