From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Kirill Shcherbatov Subject: [PATCH v3 1/6] box: rework func cache update machinery Date: Thu, 13 Jun 2019 17:08:21 +0300 Message-Id: <8c061ebeaa8f83707384004956f4429a3f4002c8.1560433806.git.kshcherbatov@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: tarantool-patches@freelists.org, vdavydov.dev@gmail.com Cc: Kirill Shcherbatov List-ID: 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(-) 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); + }); 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); }); + 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); - 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 }; 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. */ 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') -- 2.21.0