From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2ADDC24F23 for ; Thu, 24 May 2018 15:26:46 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JSq2o8Ktk-SB for ; Thu, 24 May 2018 15:26:46 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AC1B224F28 for ; Thu, 24 May 2018 15:26:45 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server References: <2baee418a3799c91c5a8b92ac62e59de3456da0d.1527084287.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4221becc-9d9c-852a-ccd1-6ed3618cde9f@tarantool.org> Date: Thu, 24 May 2018 22:26:43 +0300 MIME-Version: 1.0 In-Reply-To: <2baee418a3799c91c5a8b92ac62e59de3456da0d.1527084287.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! See 12 comments below. And I have pushed other review fixes on the branch in a separate commit. Please, look at them and squash. On 23/05/2018 17:05, Kirill Shcherbatov wrote: > New server checks stored in space_opts. For ExprList transfering > to server data is packed in msgpack as array of maps: > [{"expr_str": "x < y", "name" : "FIRST"}, ..] > Introduced checks_array_decode aimed to unpack this > complex package. > Introduced sql_checks_update_space_def_reference to update space > references as space_def pointer changes over the time, > i.e. resolved checks refer released memory. > > Resolves #3272. > --- > src/box/alter.cc | 5 +++ > src/box/opt_def.h | 5 ++- > src/box/space_def.c | 96 +++++++++++++++++++++++++++++++++++++++ > src/box/space_def.h | 12 ++--- > src/box/sql.c | 107 +++++++++++++++++++++++++++++++++++++++++++- > src/box/sql.h | 43 ++++++++++++++++++ > src/box/sql/build.c | 65 +++++++++++++++++++-------- > src/box/sql/insert.c | 33 ++++++++------ > src/box/sql/pragma.h | 2 - > src/box/sql/resolve.c | 2 - > src/box/sql/sqliteInt.h | 1 - > test/sql-tap/check.test.lua | 10 ++--- > 12 files changed, 329 insertions(+), 52 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index e4780da..b379da2 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -523,6 +523,11 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode, > space_def_new_xc(id, uid, exact_field_count, name, name_len, > engine_name, engine_name_len, &opts, fields, > field_count); > + 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())); 1. You must not reset OOM error. > diff --git a/src/box/opt_def.h b/src/box/opt_def.h > index 71fef3a..91e4f0b 100644 > --- a/src/box/opt_def.h > +++ b/src/box/opt_def.h > @@ -79,11 +79,12 @@ struct opt_def { > > #define OPT_DEF_ENUM(key, enum_name, opts, field, to_enum) \ > { key, OPT_ENUM, offsetof(opts, field), sizeof(int), #enum_name, \ > - sizeof(enum enum_name), enum_name##_strs, enum_name##_MAX, {to_enum} } > + sizeof(enum enum_name), enum_name##_strs, enum_name##_MAX, \ > + { to_enum } } 2. Garbage diff. > > #define OPT_DEF_ARRAY(key, opts, field, to_array) \ > { key, OPT_ARRAY, offsetof(opts, field), sizeof(((opts *)0)->field), \ > - NULL, 0, NULL, 0, {to_array} } > + NULL, 0, NULL, 0, { (void *)to_array} } 3. Same. And I do not see this diff on the branch. Have you sent me an out of date patch? > > #define OPT_END {NULL, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, {NULL}} > > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 9e0e7e3..7bff86f 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -33,17 +33,33 @@ > #include "diag.h" > #include "error.h" > #include "sql.h" > +#include "msgpuck.h" > + > +/** > + * Make checks from msgpack. > + * @param str pointer to array of maps > + * e.g. [{"expr_str": "x < y", "name": "ONE"}, ..]. 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'. > + * @param len array items count. > + * @param opt pointer to store parsing result. > + * @retval not NULL Checks pointer on success. > + * @retval NULL on error. > + */ > +static int > +checks_array_decode(const char **str, uint32_t len, char *opt); > > @@ -122,6 +138,19 @@ space_def_dup(const struct space_def *src) > } > } > } > + 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_destroy_fields(ret->fields, ret->field_count); > + free(ret->opts.sql); > + free(ret); > + return NULL; > + } > + sql_checks_update_space_def_reference(ret->opts.checks, ret); > + } > tuple_dictionary_ref(ret->dict); > return ret; > } > @@ -207,6 +236,18 @@ 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_destroy_fields(def->fields, def->field_count); > + free(def->opts.sql); > + free(def); > + return NULL; > + } > + sql_checks_update_space_def_reference(def->opts.checks, def); > + } 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. > return def; > } > > @@ -231,3 +272,58 @@ space_def_delete(struct space_def *def) > +static int > +checks_array_decode(const char **str, uint32_t len, char *opt) > +{ > + struct ExprList *checks = NULL; > + const char **map = str; > + struct sqlite3 *db = sql_get(); > + for (unsigned i = 0; i < len; i++) { > + checks = sql_expr_list_append(db, checks, NULL); > + if (checks == NULL) { > + diag_set(OutOfMemory, 0, "sql_expr_list_append", > + "pChecks"); > + goto error; > + } > + const char *expr_name = NULL; > + const char *expr_str = NULL; > + uint32_t expr_name_len = 0; > + uint32_t expr_str_len = 0; > + uint32_t map_size = mp_decode_map(map); > + for (unsigned j = 0; j < map_size; j++) { > + if (mp_typeof(**map) != MP_STR) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0, > + "key must be a string"); 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); > + goto error; > + } > + uint32_t key_len; > + const char *key = mp_decode_str(map, &key_len); > + if (strncmp(key, "expr_str", key_len) == 0) { > + expr_str = mp_decode_str(map, &expr_str_len); > + } else if (strncmp(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"); > + goto error; > + } 7. When you use strncmp, I can do this: 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) 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. > + } > + 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. > + if (item->zName == NULL) > + return -1; > + } > + struct Expr *check = NULL; > + if (expr_str != NULL && > + sql_expr_compile(db, expr_str, expr_str_len, > + &check) != 0) { > + sqlite3DbFree(db, item->zName); > + return -1; > + } > + item->pExpr = check; > + return 0; > +} > + > +int > +sql_checks_resolve_space_def_reference(ExprList *expr_list, > + struct space_def *def) > +{ > + Parse parser; > + sql_parser_create(&parser, sql_get()); > + parser.parse_only = true; > + > + Table dummy_table; > + memset(&dummy_table, 0, sizeof(dummy_table)); > + dummy_table.def = def; 9. Why can not you replace pTab in struct SrcList_item with space_def? > + > + sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL, > + expr_list); > + int rc = parser.rc; > + if (rc != SQLITE_OK) > + diag_set(IllegalParams, parser.zErrMsg); > + > + sql_parser_destroy(&parser); > + > + return rc == SQLITE_OK ? 0 : -1; > +} > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index d00bb1d..f5bb54d 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1848,17 +1867,14 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > return; > } > > + /* > + * 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; 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. > - > #ifndef SQLITE_OMIT_VIEW > @@ -2106,7 +2131,9 @@ sql_view_column_names(struct Parse *parse, struct Table *table) > struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select); > parse->nTab = n; > int rc = 0; > - if (table->pCheck != NULL) { > + /* Get server checks. */ > + ExprList *checks = space_checks_expr_list(table->def->id); 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? 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.