[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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.