[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server
Kirill Shcherbatov
kshcherbatov at tarantool.org
Fri May 25 14:53:48 MSK 2018
> 1. You must not reset OOM error.
@@ -1846,7 +1846,9 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
sql_parser_destroy(&parser);
if (parser.rc != SQLITE_OK) {
- diag_set(IllegalParams, parser.zErrMsg);
+ /* Error already set with diag. */
+ if (parser.rc != SQL_TARANTOOL_ERROR)
+ diag_set(IllegalParams, parser.zErrMsg);
return -1;
}
@@ -525,8 +525,13 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
if (def->opts.checks != NULL &&
sql_checks_resolve_space_def_reference(def->opts.checks,
def) != 0) {
- tnt_raise(ClientError, errcode, def->name,
- box_error_message(box_error_last()));
+ box_error_t *err = box_error_last();
+ if (box_error_code(err) != ENOMEM) {
+ tnt_raise(ClientError, errcode, def->name,
+ box_error_message(err));
+ } else {
+ diag_raise();
+ }
> 2. Garbage diff.
> 3. Same. And I do not see this diff on the branch. Have you sent me an out of
> date patch?
Yep, a little different.
> 4. I do not like 'expr_str' name. Obviously it is string, a user can not
> use another type. Sounds like "much of a muchness". Lets do like
> defaults: just 'expr
> 6. It is not index and not ER_WRONG_INDEX_OPTIONS. Here you parse space
> options. You must do like opts_parse_key - take error code and field
> number as arguments. And do diag_set(ClientError, errcode, fieldno, errmessage);
> 7. When you use strncmp, I can do this:
> I have used 'exp' instead of 'expr_str'. Strncmp is a useless function as I
> think, because it can be broken easy. Better use
> a_len == b_len && memcmp(a, b) == 0.@@ -297,19 +302,23 @@ checks_array_decode(const char **str, uint32_t len, char *opt)
uint32_t map_size = mp_decode_map(map);
for (uint32_t j = 0; j < map_size; j++) {
if (mp_typeof(**map) != MP_STR) {
- diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0,
+ diag_set(ClientError, errcode, 0,
"key must be a string");
goto error;
}
uint32_t key_len;
const char *key = mp_decode_str(map, &key_len);
- if (strncmp(key, "expr_str", key_len) == 0) {
+ if (key_len == 4 && memcmp(key, "expr", key_len) == 0) {
expr_str = mp_decode_str(map, &expr_str_len);
- } else if (strncmp(key, "name", key_len) == 0) {
+ } else if (key_len == 4 &&
+ memcmp(key, "name", key_len) == 0) {
expr_name = mp_decode_str(map, &expr_name_len);
} else {
- diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0,
- "invalid MsgPack");
+ char errmsg[DIAG_ERRMSG_MAX];
+ snprintf(errmsg, sizeof(errmsg),
+ "invalid MsgPack map field '%.*s'",
+ key_len, key);
+ diag_set(ClientError, errcode, field_no, errmsg);
goto error;
}
}
> 5. I have refactored this and the previous hunks (fixed tuple_dictionary leak in
> space_def_new). But it is bad still because of code duplicating. Looks like it is
> time to create a function like 'space_def_dup_opts(space_def a, opts b)',
> that copies `b` into `a->opts`. This function duplicates sql, checks and
> updates space_def references.
+/**
+ * Initialize def->opts with opts duplicate.
+ * @param def Def to initialize.
+ * @param opts Opts to duplicate.
+ * @retval 0 on success.
+ * @retval not 0 on error.
+ */
+static int
+space_def_dup_opts(struct space_def *def, const struct space_opts *opts)
+{
+ def->opts = *opts;
+ if (opts->sql != NULL) {
+ def->opts.sql = strdup(opts->sql);
+ if (def->opts.sql == NULL) {
+ diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup",
+ "def->opts.sql");
+ return -1;
+ }
+ }
+ if (opts->checks != NULL) {
+ def->opts.checks = sql_expr_list_dup(sql_get(), opts->checks, 0);
+ if (def->opts.checks == NULL) {
+ diag_set(OutOfMemory, 0, "sql_expr_list_dup",
+ "def->opts.checks");
+ return -1;
+ }
+ sql_checks_update_space_def_reference(def->opts.checks, def);
+ }
+ return 0;
+}
+
@@ -101,15 +135,7 @@ space_def_dup(const struct space_def *src)
return NULL;
}
memcpy(ret, src, size);
- if (src->opts.sql != NULL) {
- ret->opts.sql = strdup(src->opts.sql);
- if (ret->opts.sql == NULL) {
- diag_set(OutOfMemory, strlen(src->opts.sql) + 1,
- "strdup", "ret->opts.sql");
- free(ret);
- return NULL;
- }
- }
+ memset(&ret->opts, 0, sizeof(ret->opts));
@@ -139,16 +165,9 @@ space_def_dup(const struct space_def *src)
}
}
tuple_dictionary_ref(ret->dict);
- if (src->opts.checks != NULL) {
- ret->opts.checks =
- sql_expr_list_dup(sql_get(), src->opts.checks, 0);
- if (ret->opts.checks == NULL) {
- diag_set(OutOfMemory, 0, "sql_expr_list_dup",
- "ret->opts.checks");
- space_def_delete(ret);
- return NULL;
- }
- sql_checks_update_space_def_reference(ret->opts.checks, ret);
+ if (space_def_dup_opts(ret, &src->opts) != 0) {
+ space_def_delete(ret);
+ return NULL;
}
@@ -183,16 +202,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
def->name[name_len] = 0;
memcpy(def->engine_name, engine_name, engine_len);
def->engine_name[engine_len] = 0;
- def->opts = *opts;
- if (opts->sql != NULL) {
- def->opts.sql = strdup(opts->sql);
- if (def->opts.sql == NULL) {
- diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup",
- "def->opts.sql");
- free(def);
- return NULL;
- }
- }
+
@@ -234,15 +244,9 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
}
}
}
- if (opts->checks != NULL) {
- def->opts.checks = sql_expr_list_dup(sql_get(), opts->checks, 0);
- if (def->opts.checks == NULL) {
- diag_set(OutOfMemory, 0, "sql_expr_list_dup",
- "def->opts.checks");
- space_def_delete(def);
- return NULL;
- }
- sql_checks_update_space_def_reference(def->opts.checks, def);
+ if (space_def_dup_opts(def, opts) != 0) {
+ space_def_delete(def);
+ return NULL;
}
return def;
> box.cfg{}
> opts = {checks = {{exp = 'X<5'}}}
> format = {{name = 'X', type = 'unsigned'}}
> t = {513, 1, 'test', 'memtx', 0, opts, format}
> box.space._space:insert(t)
>
>
>> + }
>> + sql_check_list_item_init(checks, i, expr_name,
>> + expr_name_len, expr_str,
>> + expr_str_len);
>> + }
>> + *(void **)opt = checks;
>> + return 0;
>> +error:
>> + sql_expr_list_delete(db, checks);
>> + return -1;
>> +}
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 0845e65..d62e14c 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -1761,3 +1797,72 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
>> pTable->def->opts.temporary = false;
>> return 0;
>> }
>> +
>> +int
>> +sql_check_list_item_init(struct ExprList *expr_list, int idx,
>> + const char *expr_name, uint32_t expr_name_len,
>> + const char *expr_str, uint32_t expr_str_len)
>> +{
>> + assert(idx < expr_list->nExpr);
>> + struct ExprList_item *item = &expr_list->a[idx];
>> + memset(item, 0, sizeof(struct ExprList_item));
>> + if (expr_name != NULL) {
>> + item->zName = sqlite3DbStrNDup(db, expr_name, expr_name_len);
>
> 8. Please, use strndup. And set diag on OOM.
First, sqlite3DbStrNDup is not a problem in sql.c.
As ExprList is not only about checks and sql_expr_list_delete destructor releases such allocations with sqlite3DbFree I believe that such changes shouldn't going with this patch.
> 9. Why can not you replace pTab in struct SrcList_item with space_def?
This structure is required in ctx where pIndex is required. Look for sqlite3FindInIndex; sqlite3PrimaryKeyIndex in sqlite3ExprCodeIN and so on.
- if (item->zName == NULL)
+ if (item->zName == NULL) {
+ diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup",
+ "item->zName");
return -1;
+ }
> 10. deleteTable leaks with the same reason: you delete space_def only when
> opts.is_temporary flag is set. Including checks, that are on malloc.
> 11. Why do you space->def->opts.checks here? I scanned sql_view_column_names,
> and looks like it does not require compiled check. It needs only check names
> and check span. Table.def has both, it is not?
>> + /*
>> + * As rebuild creates a new ExpList tree and old_def
>> + * is allocated on region release old tree manually.
>> + */
>> + struct ExprList *old_checks = p->def->opts.checks;
>> if (sql_table_def_rebuild(db, p) != 0)
>> return;
>> + /* Get server checks. */
>> + ExprList *checks = space_checks_expr_list(table->def->id);
>
We don't replicate checks in SQL. At the end of sqlite3EndTable checks are released and the only checks collection present on the server side.
I've wrote comments about this in sqlite3EndTable:
/*
* As rebuild creates a new ExpList tree and old_def
* allocated on region release old tree manually.
*/
/*
* Checks are useless for now as all operations process with
* the server checks instance. Remove to do not consume extra memory,
* don't require make a copy on space_def_dup and to improve
* debuggability.
*/
>
> 12. Please, write tests on checks insertion from Lua. I know such checks will
> not work, but at least you should do sanity test. You should test, that a user
> can insert a check with no name, that a check can be compiled even if no SQL
> Table, that a check expr can not be number or bool and the same about name.
> In the same tests you can check errors on non-existing column names.
Fixed error handling:
diff --git a/src/box/space_def.c b/src/box/space_def.c
index d4c0c90..dda9d95 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -285,6 +285,7 @@ static int
checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
uint32_t field_no)
{
+ char errmsg[DIAG_ERRMSG_MAX];
struct ExprList *checks = NULL;
const char **map = str;
struct sqlite3 *db = sql_get();
@@ -314,7 +315,6 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
memcmp(key, "name", key_len) == 0) {
expr_name = mp_decode_str(map, &expr_name_len);
} else {
- char errmsg[DIAG_ERRMSG_MAX];
snprintf(errmsg, sizeof(errmsg),
"invalid MsgPack map field '%.*s'",
key_len, key);
@@ -322,8 +322,17 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
goto error;
}
}
- sql_check_list_item_init(checks, i, expr_name, expr_name_len,
- expr_str, expr_str_len);
+ if (sql_check_list_item_init(checks, i, expr_name, expr_name_len,
+ expr_str, expr_str_len) != 0) {
+ box_error_t *err = box_error_last();
+ if (box_error_code(err) != ENOMEM) {
+ snprintf(errmsg, sizeof(errmsg),
+ "invalid expression specified");
+ diag_set(ClientError, errcode, field_no,
+ errmsg);
+ goto error;
+ }
+ }
}
*(struct ExprList **)opt = checks;
return 0;
diff --git a/src/box/sql.c b/src/box/sql.c
index f129f0b..9abda39 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1797,8 +1797,11 @@ sql_check_list_item_init(struct ExprList *expr_list, int idx,
memset(item, 0, sizeof(*item));
if (expr_name != NULL) {
item->zName = sqlite3DbStrNDup(db, expr_name, expr_name_len);
- if (item->zName == NULL)
+ if (item->zName == NULL) {
+ diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup",
+ "item->zName");
return -1;
+ }
}
if (expr_str != NULL && sql_expr_compile(db, expr_str, expr_str_len,
&item->pExpr) != 0) {
And some tests:
+++ b/test/sql/checks.test.lua
@@ -0,0 +1,21 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+--
+-- gh-3272: Move SQL CHECK into server
+--
+
+-- invalid expression
+opts = {checks = {{expr = 'X><5'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
+opts = {checks = {{expr = 'X>5'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+box.space._space:delete(513)
+
+test_run:cmd("clear filter")
More information about the Tarantool-patches
mailing list