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 DF08224E0A for ; Wed, 16 May 2018 13:59:32 -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 dDka5WsqgoMh for ; Wed, 16 May 2018 13:59:32 -0400 (EDT) Received: from smtp14.mail.ru (smtp14.mail.ru [94.100.181.95]) (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 2D75424D8B for ; Wed, 16 May 2018 13:59:31 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v6 4/4] sql: remove Checks to server References: <4898952651385c13e1d4458e7020a23353bd4446.1526403792.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 16 May 2018 20:59:29 +0300 MIME-Version: 1.0 In-Reply-To: <4898952651385c13e1d4458e7020a23353bd4446.1526403792.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: tarantool-patches@freelists.org, Kirill Shcherbatov Hello. Thanks for the patch! See my 33 comments below. On 15/05/2018 20:03, 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_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 | 11 +++++ > src/box/key_def.cc | 2 +- > src/box/opt_def.c | 16 +++++++- > src/box/opt_def.h | 8 +++- > src/box/space_def.c | 97 ++++++++++++++++++++++++++++++++++++++++++++ > src/box/space_def.h | 6 +++ > src/box/sql.c | 83 ++++++++++++++++++++++++++++++++++++- > src/box/sql.h | 93 +++++++++++++++++++++++++++++++++++++++++- > src/box/sql/build.c | 99 ++++++++++++++++++++++++++------------------- > src/box/sql/delete.c | 6 +-- > src/box/sql/expr.c | 53 ++++++++++-------------- > src/box/sql/fkey.c | 15 +++---- > src/box/sql/insert.c | 22 +++++----- > src/box/sql/parse.y | 86 +++++++++++++++++++-------------------- > src/box/sql/pragma.h | 2 - > src/box/sql/prepare.c | 10 ++++- > src/box/sql/resolve.c | 39 ++++++------------ > src/box/sql/select.c | 45 +++++++++++---------- > src/box/sql/sqliteInt.h | 39 +++++------------- > src/box/sql/tokenize.c | 7 ++-- > src/box/sql/trigger.c | 13 +++--- > src/box/sql/update.c | 2 +- > src/box/sql/wherecode.c | 14 ++++--- > src/box/sql/whereexpr.c | 7 ++-- > test/sql-tap/check.test.lua | 13 +++--- > 25 files changed, 542 insertions(+), 246 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index c446b10..f1c2394 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -521,6 +522,16 @@ 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) { > + int rc = > + sql_resolve_checks_space_def_reference(def->opts.checks, > + def); > + if (rc != 0) > + tnt_raise(ClientError, errcode, > + tt_cstr(name, name_len), > + diag_last_error(diag_get())->errmsg); > + } > + 1. Apply this please: diff --git a/src/box/alter.cc b/src/box/alter.cc index f1c2394ac..3c6be0d45 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -522,14 +522,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) { - int rc = - sql_resolve_checks_space_def_reference(def->opts.checks, - def); - if (rc != 0) - tnt_raise(ClientError, errcode, - tt_cstr(name, name_len), - diag_last_error(diag_get())->errmsg); + if (def->opts.checks != NULL && + sql_resolve_checks_space_def_reference(def->opts.checks, + def) != 0) { + tnt_raise(ClientError, errcode, def->name, + box_error_message(box_error_last())); } > diff --git a/src/box/opt_def.h b/src/box/opt_def.h > index 633832a..d5500d7 100644 > --- a/src/box/opt_def.h > +++ b/src/box/opt_def.h > @@ -47,12 +47,14 @@ enum opt_type { > OPT_STR, /* char[] */ > OPT_STRPTR, /* char* */ > OPT_ENUM, /* enum */ > + OPT_ARRAY, /* array */ 2. Lets extract this OPT_DEF_ARRAY things into a separate commit. > opt_type_MAX, > }; > > extern const char *opt_type_strs[]; > > typedef int64_t (*opt_def_to_enum_cb)(const char *str, uint32_t len); > +typedef void *(*opt_def_action_cb)(void *data, uint32_t k); 3. As I can see, opt_def_action_cb takes MessagePack, so 'void *data' must be 'const char **data', and k is length. So rename it to 'len' please as it is done for enum. 4. I understand what happens here, but it looks too complicated. What to do, when an array member is not like ArrayType *member, but ArrayType member[fixed_count]? This will not work. > *(const char **)opt = array_parse_cb(val, ival); > if (*(const char **)opt == NULL) > return -1; Lets better pass the original 'opts' as the first argument of the collback, and make the former return int - 0 on success and -1 on error. Callback must care itself about how to store the array. > > struct opt_def { > const char *name; > @@ -65,7 +67,7 @@ struct opt_def { > const char **enum_strs; > uint32_t enum_max; > /** If not NULL, used to get a enum value by a string. */ > - opt_def_to_enum_cb to_enum; > + void *callback; 5. Lets create a second typedef with array callback signature, and put here both enum and array callbacks in union. This allow you to do not cast void * to function pointer. > struct region; > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 1fa3345..2dbcac5 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -29,21 +29,29 @@ > * SUCH DAMAGE. > */ > > +#include "sqliteInt.h" 6. Please, remove dependency on sqliteInt.h. All that you need must be in sql.h. > #include "space_def.h" > #include "diag.h" > #include "error.h" > #include "sql.h" > +#include "msgpuck.h" 7. Why? I removed this and nothing is changed. > @@ -123,6 +131,18 @@ 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"); > + free(ret->opts.sql); > + free(ret); 8. You did not destroy default_value expressions built above. > + return NULL; > + } > + sql_update_checks_space_def_reference(ret->opts.checks, ret); 9. Lets all CHECK methods name sql_checks_. Here it will be sql_checks_update_space_def_reference. > @@ -209,6 +229,16 @@ 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.pCheck"); > + free(def); 10. Same as 8. > + return NULL; > + } > + sql_update_checks_space_def_reference(def->opts.checks, def); > + } > return def; > } > > @@ -233,3 +263,70 @@ space_def_delete(struct space_def *def) > TRASH(def); > free(def); > } > + > +/** > + * Decode checks from msgpack. > + * @param data pointer to array of maps > + * e.g. [{"expr_str": "x < y", "name": "ONE"}, ..]. > + * @param array_items array items count. > + * @retval not NULL Checks pointer on success. > + * @retval NULL on error. > + */ > +void * > +checks_array_decode(void *data, uint32_t array_items) 11. I saw that you moved this out of space_def.c, but why? It is space_opts decoder, and must be in space_def.c file, it is not? > +{ > + struct ExprList *pChecks = NULL; > + const char **map = (const char **)data; > + struct sqlite3 *db = sql_get(); > + for (unsigned i = 0; i < array_items; i++) { > + pChecks = sql_expr_list_append(db, pChecks, NULL); > + if (pChecks == NULL) { > + diag_set(OutOfMemory, 0, "sql_expr_list_append", > + "pChecks"); > + goto error; 12. Here you go to error to free pChecks, but it is NULL already. > + } > + struct ExprList_item *pItem = > + &pChecks->a[pChecks->nExpr - 1]; > + 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"); > + goto error; > + } > + uint32_t key_len; > + const char *key = mp_decode_str(map, &key_len); > + if (strncmp(key, "expr_str", key_len) == 0) { > + uint32_t expr_str_len = 0; > + const char *expr_str = > + mp_decode_str(map, &expr_str_len); > + struct Expr *check_expr = NULL; > + if (sql_expr_compile(db, expr_str, expr_str_len, > + &check_expr) != 0) > + goto error; > + pItem->pExpr = check_expr; > + } else if (strncmp(key, "name", key_len) == 0) { > + uint32_t name_str_len = 0; > + const char *name_str = > + mp_decode_str(map, &name_str_len); > + assert(pItem->zName == NULL); > + pItem->zName = sqlite3DbStrNDup(db, name_str, > + name_str_len); 13. Lets do not use DB allocations for memory, that is stored in the core. > diff --git a/src/box/space_def.h b/src/box/space_def.h > index 52447b6..0d2b9e6 100644 > --- a/src/box/space_def.h > +++ b/src/box/space_def.h > @@ -33,6 +33,7 @@ > #include "trivia/util.h" > #include "tuple_dictionary.h" > #include "schema_def.h" > +#include "sql.h" 14. Do not include sql.h. You can predeclare struct ExprList, and move space_opts_destroy into space_def.c. > diff --git a/src/box/sql.c b/src/box/sql.c > index 357cbf9..bdb7e5b 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1510,13 +1510,45 @@ int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf) > bool is_view = false; > if (pTable != NULL) > is_view = pTable->def->opts.is_view; > - p = enc->encode_map(base, is_view ? 2 : 1); > + bool has_checks = (pTable != NULL && pTable->def->opts.checks != NULL && > + pTable->def->opts.checks->nExpr > 0); 15. How is it possible, that checks != NULL, but checks.nExpr == 0? > + int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0; > + > + int map_fields = 1; > + map_fields += (is_view == true); 16. Why do you need () ? > + map_fields += (has_checks == true); > + p = enc->encode_map(base, map_fields); > p = enc->encode_str(p, "sql", 3); > p = enc->encode_str(p, zSql, strlen(zSql)); > if (is_view) { > p = enc->encode_str(p, "view", 4); > p = enc->encode_bool(p, true); > } > + > + if (!has_checks || checks_cnt == 0) 17. Why is not has_checks the same as checks_cnt > 0 ? > + return (int)(p - base); > + > + struct ExprList_item *a = pTable->def->opts.checks->a; > + p = enc->encode_str(p, "checks", 6); > + p = enc->encode_array(p, checks_cnt); > + for (int i = 0; i < checks_cnt; ++i) { > + int items = 0; > + items += (a[i].pExpr != NULL); > + items += (a[i].zName != NULL); 18. Same as 16. > + p = enc->encode_map(p, items); > + if (a[i].pExpr != NULL) { 19. How is it possible, that pExpr == NULL in a check? > @@ -1739,6 +1771,52 @ > int > sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) > { > @@ -1755,5 +1833,8 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) > } > pTable->def = new_def; > pTable->def->opts.temporary = false; > + if (new_def->opts.checks != NULL) > + sql_update_checks_space_def_reference(new_def->opts.checks, > + new_def); 20. You already do this in space_def_new. > diff --git a/src/box/sql.h b/src/box/sql.h > index 3c26492..6399c79 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -74,12 +74,14 @@ struct Table; > * stuct Select and return it. > * @param db SQL context handle. > * @param expr Expression to parse. > + * @param expr_len Expression to parse length (optional). 21. Optional? Looks like not. > * @param[out] result Result: AST structure. > * > * @retval Error code if any. > */ > int > -sql_expr_compile(struct sqlite3 *db, const char *expr, struct Expr **result); > +sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, > + struct Expr **result); > > @@ -175,6 +177,95 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name); > + > +/** > + * Add a new element to the end of an expression list. If pList is 22. What is pList? > + * initially NULL, then create a new expression list. > + * > + * If a memory allocation error occurs, the entire list is freed and > + * NULL is returned. If non-NULL is returned, then it is guaranteed > + * that the new entry was successfully appended. > + * @param db SQL handle. > + * @param expr_list List to which to append. Might be NULL. > + * @param expr Expression to be appended. Might be NULL. > + * @retval NULL on memory allocation error. > + * @retval not NULL on success. > + */ > +struct ExprList * > +sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list, struct Expr *expr); > + > +/** > + * Resolve names in expressions that can only reference a single table: 23. Too many white spaces after *. And below too. > + * * CHECK constraints > + * * WHERE clauses on partial indices > + * The Expr.iTable value for Expr.op==TK_COLUMN nodes of the expression > + * is set to -1 and the Expr.iColumn value is set to the column number. > + * Any errors cause an error message to be set in pParse. > + * @param pParse Parsing context. > + * @param pTab The table being referenced. > + * @param type NC_IsCheck or NC_PartIdx or NC_IdxExpr. > + * @param pExpr Expression to resolve. May be NULL. > + * @param pList Expression list to resolve. May be NUL. 24. Names are out of date. > + */ > +void > +sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type, > + struct Expr *expr, struct ExprList *expr_list); > + > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 718809d..60f1a50 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1964,6 +1973,12 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > } > #endif > } > + > + /* We don't need Checks in SQL anymore. */ > + if (p->def->opts.checks != NULL) { > + sql_expr_list_free(db, p->def->opts.checks); > + p->def->opts.checks = NULL; 25. space_opts_destroy must do it, when Table object is destroyed. > @@ -2100,7 +2115,8 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > db->lookaside.bDisable++; > pSelTab = sqlite3ResultSetOfSelect(pParse, pSel); > pParse->nTab = n; > - if (pTable->pCheck) { > + ExprList *checks = space_checks_expr_list(pTable->def->id); 26. Why you can not get checks from def->opts.checks? > @@ -2562,8 +2577,8 @@ sqlite3CreateForeignKey(Parse * pParse, /* Parsing context */ > fk_end: > sqlite3DbFree(db, pFKey); > #endif /* !defined(SQLITE_OMIT_FOREIGN_KEY) */ > - sqlite3ExprListDelete(db, pFromCol); > - sqlite3ExprListDelete(db, pToCol); > + sql_expr_list_free(db, pFromCol); > + sql_expr_list_free(db, pToCol); 27. The previous name was correct - delete, not free. Our naming convention is new/delete for allocating and freeing objects, and create/destroy for initializing and cleaning an object. > @@ -2971,7 +2986,7 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ > struct field_def *field = > &pTab->def->fields[pTab->def->field_count - 1]; > sqlite3TokenInit(&prevCol, field->name); > - pList = sqlite3ExprListAppend(pParse, 0, > + pList = sql_expr_list_append(pParse->db, 0, > sqlite3ExprAlloc(db, TK_ID, 28. Now the indentation is wrong. > @@ -3024,8 +3039,8 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ > pIndex->nColumn = pList->nExpr; > /* Tarantool have access to each column by any index */ > if (pPIWhere) { > - sqlite3ResolveSelfReference(pParse, pTab, NC_PartIdx, pPIWhere, > - 0); > + sql_resolve_self_reference(pParse, pTab, NC_PartIdx, pPIWhere, > + 0); 29. NULL, not 0. Last argument is a pointer. In other places too, please. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 1a883e2..32ad806 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -922,7 +922,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > > insert_cleanup: > sqlite3SrcListDelete(db, pTabList); > - sqlite3ExprListDelete(db, pList); > + sql_expr_list_free(db, pList); > sqlite3SelectDelete(db, pSelect); > sqlite3IdListDelete(db, pColumn); > sqlite3DbFree(db, aRegIdx); > @@ -1183,10 +1183,10 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */ > > /* Test all CHECK constraints > */ > -#ifndef SQLITE_OMIT_CHECK > - if (pTab->pCheck && (user_session->sql_flags & > + uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum); > + ExprList *pCheck = space_checks_expr_list(space_id); 30. Why you can not get checks from pTab->def->opts.check? > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index b078e20..b7f2f45 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -286,7 +286,7 @@ ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). > {sqlite3AddPrimaryKey(pParse,0,R,I,Z);} > ccons ::= UNIQUE onconf(R). {sqlite3CreateIndex(pParse,0,0,0,R,0,0,0,0, > SQLITE_IDXTYPE_UNIQUE);} > -ccons ::= CHECK LP expr(X) RP. {sqlite3AddCheckConstraint(pParse,X.pExpr);} > +ccons ::= CHECK LP expr(X) RP. {sqlite3AddCheckConstraint(pParse,X.pExpr,&X);} 31. I see, that in all sqlite3AddCheckConstraint usage places you pass ExprSpan and its pExpr field separately. Why? You can pass ExprSpan and get its pExpr field inside sqlite3AddCheckConstraint. > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index bf729b7..3c2cd63 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -530,10 +530,8 @@ notValid(Parse * pParse, /* Leave error message here */ > const char *zIn = "partial index WHERE clauses"; > if (pNC->ncFlags & NC_IdxExpr) > zIn = "index expressions"; > -#ifndef SQLITE_OMIT_CHECK 32. I see, that you try to delete SQLITE_OMIT_CHECK. It still exists in build.c. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 75699ae..44897af 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -1911,7 +1911,6 @@ struct Table { > Select *pSelect; /* NULL for tables. Points to definition if a view. */ > FKey *pFKey; /* Linked list of all foreign keys in this table */ > char *zColAff; /* String defining the affinity of each column */ > - ExprList *pCheck; /* All CHECK constraints */ > /* ... also used as column name list in a VIEW */ > Hash idxHash; /* All (named) indices indexed by name */ > int tnum; /* Root BTree page for this table */ > @@ -2894,10 +2893,8 @@ struct Parse { > Token constraintName; /* Name of the constraint currently being parsed */ > int regRoot; /* Register holding root page number for new objects */ > int nMaxArg; /* Max args passed to user function by sub-program */ > -#ifdef SELECTTRACE_ENABLED 33. Can you extract this and sql_parser_create() moving into a little separate commit?