From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Kirill Shcherbatov Subject: Re: [tarantool-patches] Re: [PATCH v3 1/6] box: rework func cache update machinery References: <8c061ebeaa8f83707384004956f4429a3f4002c8.1560433806.git.kshcherbatov@tarantool.org> <20190618105250.wv5igt3imhsxkq77@esperanza> Message-ID: Date: Wed, 19 Jun 2019 18:51:55 +0300 MIME-Version: 1.0 In-Reply-To: <20190618105250.wv5igt3imhsxkq77@esperanza> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Vladimir Davydov List-ID: Done. ========================================================= 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 | 60 +++++++++++++++++++----------------- src/box/errcode.h | 2 +- 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 | 30 ++++++------------ src/box/schema.h | 9 ++---- test/box/function1.result | 4 +++ test/box/function1.test.lua | 1 + test/box/misc.result | 1 - test/wal_off/func_max.result | 4 +-- 12 files changed, 81 insertions(+), 70 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index a37a68ce4..adad957f2 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2550,6 +2550,10 @@ func_def_new_from_tuple(struct tuple *tuple) tnt_raise(OutOfMemory, func_def_sizeof(len), "malloc", "def"); auto def_guard = make_scoped_guard([=] { free(def); }); func_def_get_ids_from_tuple(tuple, &def->fid, &def->uid); + if (def->fid > BOX_FUNCTION_MAX) { + tnt_raise(ClientError, ER_CREATE_FUNCTION, + tt_cstr(name, len), "function id is too big"); + } memcpy(def->name, name, len); def->name[len] = 0; if (tuple_field_count(tuple) > BOX_FUNC_FIELD_SETUID) @@ -2574,24 +2578,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) -{ - 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) +func_cache_remove_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 +2603,17 @@ 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); - def_guard.is_active = false; struct trigger *on_rollback = txn_alter_trigger_new(func_cache_remove_func, NULL); + struct func *func = func_new(def); + if (func == NULL) + diag_raise(); + def_guard.is_active = false; + func_cache_insert(func); + on_rollback->data = func; txn_on_rollback(txn, on_rollback); } else if (new_tuple == NULL) { /* DELETE */ uint32_t uid; @@ -2636,16 +2631,25 @@ 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 *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/errcode.h b/src/box/errcode.h index 28d438333..55299b735 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -106,7 +106,7 @@ struct errcode_record { /* 51 */_(ER_NO_SUCH_FUNCTION, "Function '%s' does not exist") \ /* 52 */_(ER_FUNCTION_EXISTS, "Function '%s' already exists") \ /* 53 */_(ER_BEFORE_REPLACE_RET, "Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \ - /* 54 */_(ER_FUNCTION_MAX, "A limit on the total number of functions has been reached: %u") \ + /* 54 */_(ER_UNUSED2, "") \ /* 55 */_(ER_TRIGGER_EXISTS, "Trigger '%s' already exists") \ /* 56 */_(ER_USER_MAX, "A limit on the total number of users has been reached: %u") \ /* 57 */_(ER_NO_SUCH_ENGINE, "Space engine '%s' does not exist") \ 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..d63add535 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -528,6 +528,7 @@ schema_free(void) struct func *func = ((struct func *) mh_i32ptr_node(funcs, i)->val); func_cache_delete(func->def->fid); + func_delete(func); } mh_i32ptr_delete(funcs); while (mh_size(sequences) > 0) { @@ -541,39 +542,27 @@ 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 }; + assert(func_by_id(func->def->fid) == NULL); + assert(func_by_name(func->def->name, strlen(func->def->name)) == NULL); + const struct mh_i32ptr_node_t node = { func->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; + panic_syserror("Out of memory for the data dictionary cache " + "(stored function)."); } 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); if (k2 == mh_end(funcs_by_name)) { mh_i32ptr_del(funcs, k1, NULL); - func->def = NULL; func_delete(func); - goto error; + panic_syserror("Out of memory for the data dictionary cache " + "(stored function)."); } } @@ -590,7 +579,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..7e8ac6c11 100644 --- a/src/box/schema.h +++ b/src/box/schema.h @@ -163,14 +163,11 @@ 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. */ 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') diff --git a/test/box/misc.result b/test/box/misc.result index 43b5a4a15..0b3902304 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -384,7 +384,6 @@ t; 51: box.error.NO_SUCH_FUNCTION 52: box.error.FUNCTION_EXISTS 53: box.error.BEFORE_REPLACE_RET - 54: box.error.FUNCTION_MAX 55: box.error.TRIGGER_EXISTS 56: box.error.USER_MAX 57: box.error.NO_SUCH_ENGINE diff --git a/test/wal_off/func_max.result b/test/wal_off/func_max.result index 5a43821b2..ab4217845 100644 --- a/test/wal_off/func_max.result +++ b/test/wal_off/func_max.result @@ -42,7 +42,7 @@ test_run:cmd("setopt delimiter ''"); ... func_limit() --- -- error: 'A limit on the total number of functions has been reached: 32000' +- error: 'Failed to create function ''func32000'': function id is too big' ... drop_limit_func() --- @@ -62,7 +62,7 @@ session.su('testuser') ... func_limit() --- -- error: 'A limit on the total number of functions has been reached: 32000' +- error: 'Failed to create function ''func32000'': function id is too big' ... drop_limit_func() --- -- 2.21.0