From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 18 Jun 2019 13:52:50 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 1/6] box: rework func cache update machinery Message-ID: <20190618105250.wv5igt3imhsxkq77@esperanza> References: <8c061ebeaa8f83707384004956f4429a3f4002c8.1560433806.git.kshcherbatov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8c061ebeaa8f83707384004956f4429a3f4002c8.1560433806.git.kshcherbatov@tarantool.org> To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Jun 13, 2019 at 05:08:21PM +0300, Kirill Shcherbatov wrote: > Tarantool used to assume that func_new call must not fail and > it used to build a new func object by given definition just on > func cache replace operation. We need to fix it to perform > user-dependent risky actions like Lua function assemble in > further patches. > > The replace method is disallowed for _func space because it is > redundant and difficult to maintain in case of functions that > have pre-compiled runtime. > > Needed for #4182, #1260 > --- > src/box/alter.cc | 57 +++++++++++++++++++------------------ > src/box/func.c | 8 ------ > src/box/func.h | 3 -- > src/box/func_def.c | 17 +++++++++++ > src/box/func_def.h | 12 ++++++++ > src/box/schema.cc | 37 +++++++++--------------- > src/box/schema.h | 10 +++---- > test/box/function1.result | 4 +++ > test/box/function1.test.lua | 1 + > 9 files changed, 81 insertions(+), 68 deletions(-) Generally, this patch is fine. See a few very minor comments below. > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index a37a68ce4..18b50a5da 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -2574,24 +2574,11 @@ func_def_new_from_tuple(struct tuple *tuple) > > /** Remove a function from function cache */ > static void > -func_cache_remove_func(struct trigger * /* trigger */, void *event) > +func_cache_remove_func(struct trigger *trigger, void * /* event */) > { > - struct txn_stmt *stmt = txn_last_stmt((struct txn *) event); > - uint32_t fid = tuple_field_u32_xc(stmt->old_tuple ? > - stmt->old_tuple : stmt->new_tuple, > - BOX_FUNC_FIELD_ID); > - func_cache_delete(fid); > -} > - > -/** Replace a function in the function cache */ > -static void > -func_cache_replace_func(struct trigger * /* trigger */, void *event) > -{ > - struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); > - struct func_def *def = func_def_new_from_tuple(stmt->new_tuple); > - auto def_guard = make_scoped_guard([=] { free(def); }); > - func_cache_replace(def); > - def_guard.is_active = false; > + struct func *old_func = (struct func *) trigger->data; > + func_cache_delete(old_func->def->fid); > + func_delete(old_func); > } > > /** > @@ -2612,13 +2599,21 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) > struct func *old_func = func_by_id(fid); > if (new_tuple != NULL && old_func == NULL) { /* INSERT */ > struct func_def *def = func_def_new_from_tuple(new_tuple); > + auto def_guard = make_scoped_guard([=] { free(def); }); > access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION, > PRIV_C); > - auto def_guard = make_scoped_guard([=] { free(def); }); > - func_cache_replace(def); > + struct func *func = func_new(def); > + if (func == NULL) > + diag_raise(); > + auto func_guard = make_scoped_guard([=] { > + func_cache_delete(def->fid); > + func_delete(func); > + }); Nit: I would make func_cache_insert exception-free (see my comment below), and move on_rollback trigger allocation upper. This way we wouldn't need the func_guard at all. > def_guard.is_active = false; > + func_cache_insert(func); > + func_guard.is_active = false; > struct trigger *on_rollback = > - txn_alter_trigger_new(func_cache_remove_func, NULL); > + txn_alter_trigger_new(func_cache_remove_func, func); > txn_on_rollback(txn, on_rollback); > } else if (new_tuple == NULL) { /* DELETE */ > uint32_t uid; > @@ -2636,16 +2631,22 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) > "function has grants"); > } > struct trigger *on_commit = > - txn_alter_trigger_new(func_cache_remove_func, NULL); > + txn_alter_trigger_new(func_cache_remove_func, old_func); > txn_on_commit(txn, on_commit); > } else { /* UPDATE, REPLACE */ > - struct func_def *def = func_def_new_from_tuple(new_tuple); > - auto def_guard = make_scoped_guard([=] { free(def); }); > - access_check_ddl(def->name, def->fid, def->uid, SC_FUNCTION, > - PRIV_A); > - struct trigger *on_commit = > - txn_alter_trigger_new(func_cache_replace_func, NULL); > - txn_on_commit(txn, on_commit); > + assert(new_tuple != NULL && old_tuple != NULL); > + /** > + * Allow an alter that doesn't change the > + * definition to support upgrade script. > + */ > + struct func_def *new_def = func_def_new_from_tuple(new_tuple); > + auto new_def_guard = make_scoped_guard([=] { free(new_def); }); > + struct func_def *old_def = func_def_new_from_tuple(old_tuple); > + auto old_def_guard = make_scoped_guard([=] { free(old_def); }); Nit: one guard should be enough, I think: struct func_def *old_def = NULL, *new_def = NULL; auto guard = make_scoped_guard([=] { free(old_def); free(new_def); }); old_def = func_def_new_from_tuple(old_tuple); new_def = func_def_new_from_tuple(new_tuple); > + if (func_def_cmp(new_def, old_def) != 0) { > + tnt_raise(ClientError, ER_UNSUPPORTED, "function", > + "alter"); > + } > } > } > > diff --git a/src/box/func.c b/src/box/func.c > index a817851fd..d1b8879ad 100644 > --- a/src/box/func.c > +++ b/src/box/func.c > @@ -451,14 +451,6 @@ func_call(struct func *func, box_function_ctx_t *ctx, const char *args, > return rc; > } > > -void > -func_update(struct func *func, struct func_def *def) > -{ > - func_unload(func); > - free(func->def); > - func->def = def; > -} > - > void > func_delete(struct func *func) > { > diff --git a/src/box/func.h b/src/box/func.h > index 8dcd61d7b..fa4d738ab 100644 > --- a/src/box/func.h > +++ b/src/box/func.h > @@ -100,9 +100,6 @@ module_free(void); > struct func * > func_new(struct func_def *def); > > -void > -func_update(struct func *func, struct func_def *def); > - > void > func_delete(struct func *func); > > diff --git a/src/box/func_def.c b/src/box/func_def.c > index 76ed77b24..2b135e2d7 100644 > --- a/src/box/func_def.c > +++ b/src/box/func_def.c > @@ -1,3 +1,20 @@ > #include "func_def.h" > +#include "string.h" > > const char *func_language_strs[] = {"LUA", "C"}; > + > +int > +func_def_cmp(struct func_def *def1, struct func_def *def2) > +{ > + if (def1->fid != def2->fid) > + return def1->fid - def2->fid; > + if (def1->uid != def2->uid) > + return def1->uid - def2->uid; > + if (def1->setuid != def2->setuid) > + return def1->setuid - def2->setuid; > + if (def1->language != def2->language) > + return def1->language - def2->language; > + if (strcmp(def1->name, def2->name) != 0) > + return strcmp(def1->name, def2->name); > + return 0; > +} > diff --git a/src/box/func_def.h b/src/box/func_def.h > index 5b52ab498..4c9738aea 100644 > --- a/src/box/func_def.h > +++ b/src/box/func_def.h > @@ -34,6 +34,10 @@ > #include "trivia/util.h" > #include > > +#ifdef __cplusplus > +extern "C" { > +#endif > + > /** > * The supported language of the stored function. > */ > @@ -79,6 +83,10 @@ func_def_sizeof(uint32_t name_len) > return sizeof(struct func_def) + name_len + 1; > } > > +/** Compare two given function definitions. */ > +int > +func_def_cmp(struct func_def *def1, struct func_def *def2); > + > /** > * API of C stored function. > */ > @@ -86,4 +94,8 @@ typedef struct box_function_ctx box_function_ctx_t; > typedef int (*box_function_f)(box_function_ctx_t *ctx, > const char *args, const char *args_end); > > +#ifdef __cplusplus > +} > +#endif > + > #endif /* TARANTOOL_BOX_FUNC_DEF_H_INCLUDED */ > diff --git a/src/box/schema.cc b/src/box/schema.cc > index 6d3153815..f834610a5 100644 > --- a/src/box/schema.cc > +++ b/src/box/schema.cc > @@ -541,40 +541,32 @@ schema_free(void) > } > > void > -func_cache_replace(struct func_def *def) > +func_cache_insert(struct func *func) > { > - struct func *old = func_by_id(def->fid); > - if (old) { > - func_update(old, def); > - return; > - } > if (mh_size(funcs) >= BOX_FUNCTION_MAX) > tnt_raise(ClientError, ER_FUNCTION_MAX, BOX_FUNCTION_MAX); Nit: let's also move this check to func_def_new_from_tuple. This would consistent with space_def_new_from_tuple and it would make func_cache_insert protocol more straighforward - panic on OOM, otherwise exception-free. > - struct func *func = func_new(def); > - if (func == NULL) { > -error: > - panic_syserror("Out of memory for the data " > - "dictionary cache (stored function)."); > - } > - const struct mh_i32ptr_node_t node = { def->fid, func }; > - mh_int_t k1 = mh_i32ptr_put(funcs, &node, NULL, NULL); > - if (k1 == mh_end(funcs)) { > - func->def = NULL; > - func_delete(func); > - goto error; > - } > + > + mh_int_t k1, k2; > + struct mh_i32ptr_node_t old_node, *old_node_ptr = &old_node; > + const struct mh_i32ptr_node_t node = { func->def->fid, func }; Nit: I wouldn't change this code. Instead I would add to the beginning of this function: assert(func_by_id(func->def->id) == NULL); assert(func_by_name(func->def->name, func->def->name_len) == NULL); > size_t def_name_len = strlen(func->def->name); > uint32_t name_hash = mh_strn_hash(func->def->name, def_name_len); > const struct mh_strnptr_node_t strnode = { > func->def->name, def_name_len, name_hash, func }; > > - mh_int_t k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL); > + k1 = mh_i32ptr_put(funcs, &node, &old_node_ptr, NULL); > + if (k1 == mh_end(funcs)) > + goto error; > + assert(old_node_ptr == NULL); > + k2 = mh_strnptr_put(funcs_by_name, &strnode, NULL, NULL); > if (k2 == mh_end(funcs_by_name)) { > mh_i32ptr_del(funcs, k1, NULL); > - func->def = NULL; > - func_delete(func); > goto error; > } > + return; > +error: > + panic_syserror("Out of memory for the data dictionary cache " > + "(stored function)."); > } > > void > @@ -590,7 +582,6 @@ func_cache_delete(uint32_t fid) > strlen(func->def->name)); > if (k != mh_end(funcs)) > mh_strnptr_del(funcs_by_name, k, NULL); > - func_delete(func); > } > > struct func * > diff --git a/src/box/schema.h b/src/box/schema.h > index 6f9a96117..d7b3c4c2d 100644 > --- a/src/box/schema.h > +++ b/src/box/schema.h > @@ -163,14 +163,12 @@ schema_free(); > struct space *schema_space(uint32_t id); > > /** > - * Insert a new function or update the old one. > - * > - * @param def Function definition. In a case of success the ownership > - * of @a def is transfered to the data dictionary, thus the caller > - * must not delete it. > + * Insert a new function object in the function cache. > + * @param func Function object to insert. > + * @retval Returns 0 on success, -1 otherwise. Nit: there's no return value :-) It would be nice to mention that there must not be a function with the same name or id present in the cache. > */ > void > -func_cache_replace(struct func_def *def); > +func_cache_insert(struct func *func); > > void > func_cache_delete(uint32_t fid); > diff --git a/test/box/function1.result b/test/box/function1.result > index 5b04fa97d..cadeb0467 100644 > --- a/test/box/function1.result > +++ b/test/box/function1.result > @@ -16,6 +16,10 @@ c = net.connect(os.getenv("LISTEN")) > box.schema.func.create('function1', {language = "C"}) > --- > ... > +box.space._func:replace{2, 1, 'function1', 0, 'LUA'} > +--- > +- error: function does not support alter > +... > box.schema.user.grant('guest', 'execute', 'function', 'function1') > --- > ... > diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua > index 855dd5382..e983495b6 100644 > --- a/test/box/function1.test.lua > +++ b/test/box/function1.test.lua > @@ -7,6 +7,7 @@ net = require('net.box') > c = net.connect(os.getenv("LISTEN")) > > box.schema.func.create('function1', {language = "C"}) > +box.space._func:replace{2, 1, 'function1', 0, 'LUA'} > box.schema.user.grant('guest', 'execute', 'function', 'function1') > _ = box.schema.space.create('test') > _ = box.space.test:create_index('primary')