[tarantool-patches] Re: [PATCH v3 1/6] box: rework func cache update machinery
Kirill Shcherbatov
kshcherbatov at tarantool.org
Wed Jun 19 18:51:55 MSK 2019
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 <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..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
More information about the Tarantool-patches
mailing list