[PATCH v3 1/6] box: rework func cache update machinery
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jun 18 13:52:50 MSK 2019
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 <stdbool.h>
>
> +#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')
More information about the Tarantool-patches
mailing list