[tarantool-patches] Re: [PATCH v6 4/4] sql: remove Checks to server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 16 20:59:29 MSK 2018
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_<method_name>.
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?
More information about the Tarantool-patches
mailing list