[tarantool-patches] Re: [PATCH v6 4/4] sql: remove Checks to server

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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?