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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 1. You must not reset OOM error.
@@ -1846,7 +1846,9 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
 
        sql_parser_destroy(&parser);
        if (parser.rc != SQLITE_OK) {
-               diag_set(IllegalParams, parser.zErrMsg);
+               /* Error already set with diag. */
+               if (parser.rc != SQL_TARANTOOL_ERROR)
+                       diag_set(IllegalParams, parser.zErrMsg);
                return -1;
        }


@@ -525,8 +525,13 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
        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()));
+               box_error_t *err = box_error_last();
+               if (box_error_code(err) != ENOMEM) {
+                       tnt_raise(ClientError, errcode, def->name,
+                                 box_error_message(err));
+               } else {
+                       diag_raise();
+               }

> 2. Garbage diff.
> 3. Same. And I do not see this diff on the branch. Have you sent me an out of
> date patch?
Yep, a little different.

> 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
> 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);
> 7. When you use strncmp, I can do this:
> 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.@@ -297,19 +302,23 @@ checks_array_decode(const char **str, uint32_t len, char *opt)
                uint32_t map_size = mp_decode_map(map);
                for (uint32_t j = 0; j < map_size; j++) {
                        if (mp_typeof(**map) != MP_STR) {
-                               diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, 0,
+                               diag_set(ClientError, errcode, 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) {
+                       if (key_len == 4 && memcmp(key, "expr", key_len) == 0) {
                                expr_str = mp_decode_str(map, &expr_str_len);
-                       } else if (strncmp(key, "name", key_len) == 0) {
+                       } else if (key_len == 4 &&
+                                  memcmp(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");
+                               char errmsg[DIAG_ERRMSG_MAX];
+                               snprintf(errmsg, sizeof(errmsg),
+                                        "invalid MsgPack map field '%.*s'",
+                                        key_len, key);
+                               diag_set(ClientError, errcode, field_no, errmsg);
                                goto error;
                        }
                }


> 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.

+/**
+ * Initialize def->opts with opts duplicate.
+ * @param def  Def to initialize.
+ * @param opts Opts to duplicate.
+ * @retval 0 on success.
+ * @retval not 0 on error.
+ */
+static int
+space_def_dup_opts(struct space_def *def, const struct space_opts *opts)
+{
+       def->opts = *opts;
+       if (opts->sql != NULL) {
+               def->opts.sql = strdup(opts->sql);
+               if (def->opts.sql == NULL) {
+                       diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup",
+                                "def->opts.sql");
+                       return -1;
+               }
+       }
+       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");
+                       return -1;
+               }
+               sql_checks_update_space_def_reference(def->opts.checks, def);
+       }
+       return 0;
+}
+

@@ -101,15 +135,7 @@ space_def_dup(const struct space_def *src)
                return NULL;
        }
        memcpy(ret, src, size);
-       if (src->opts.sql != NULL) {
-               ret->opts.sql = strdup(src->opts.sql);
-               if (ret->opts.sql == NULL) {
-                       diag_set(OutOfMemory, strlen(src->opts.sql) + 1,
-                                "strdup", "ret->opts.sql");
-                       free(ret);
-                       return NULL;
-               }
-       }
+       memset(&ret->opts, 0, sizeof(ret->opts));

@@ -139,16 +165,9 @@ space_def_dup(const struct space_def *src)
                }
        }
        tuple_dictionary_ref(ret->dict);
-       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_delete(ret);
-                       return NULL;
-               }
-               sql_checks_update_space_def_reference(ret->opts.checks, ret);
+       if (space_def_dup_opts(ret, &src->opts) != 0) {
+               space_def_delete(ret);
+               return NULL;
        }

@@ -183,16 +202,7 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
        def->name[name_len] = 0;
        memcpy(def->engine_name, engine_name, engine_len);
        def->engine_name[engine_len] = 0;
-       def->opts = *opts;
-       if (opts->sql != NULL) {
-               def->opts.sql = strdup(opts->sql);
-               if (def->opts.sql == NULL) {
-                       diag_set(OutOfMemory, strlen(opts->sql) + 1, "strdup",
-                                "def->opts.sql");
-                       free(def);
-                       return NULL;
-               }
-       }
+

@@ -234,15 +244,9 @@ 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_delete(def);
-                       return NULL;
-               }
-               sql_checks_update_space_def_reference(def->opts.checks, def);
+       if (space_def_dup_opts(def, opts) != 0) {
+               space_def_delete(def);
+               return NULL;
        }
        return def;


> 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)
> 

> 
>> +		}
>> +		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.
First, sqlite3DbStrNDup is not a problem in sql.c. 
As ExprList is not only about checks and sql_expr_list_delete destructor releases such allocations with sqlite3DbFree I believe that such changes shouldn't going with this patch.

> 9. Why can not you replace pTab in struct SrcList_item with space_def?
This structure is required in ctx where pIndex is required. Look for sqlite3FindInIndex; sqlite3PrimaryKeyIndex in sqlite3ExprCodeIN and so on.

-               if (item->zName == NULL)
+               if (item->zName == NULL) {
+                       diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup",
+                                "item->zName");
                        return -1;
+               }

> 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.
> 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?
>> +	/*
>> +	 * 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;

>> +	/* Get server checks. */
>> +	ExprList *checks = space_checks_expr_list(table->def->id);
> 
We don't replicate checks in SQL. At the end of sqlite3EndTable checks are released and the only checks collection present on the server side.
I've wrote comments about this in  sqlite3EndTable:

/*
* As rebuild creates a new ExpList tree and old_def
* allocated on region release old tree manually.
*/
/*
* Checks are useless for now as all operations process with
 * the server checks instance. Remove to do not consume extra memory,
 * don't require make a copy on space_def_dup and to improve
 * debuggability.
 */
> 
> 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.

Fixed error handling:

diff --git a/src/box/space_def.c b/src/box/space_def.c
index d4c0c90..dda9d95 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -285,6 +285,7 @@ static int
 checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
 		    uint32_t field_no)
 {
+	char errmsg[DIAG_ERRMSG_MAX];
 	struct ExprList *checks = NULL;
 	const char **map = str;
 	struct sqlite3 *db = sql_get();
@@ -314,7 +315,6 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
 				   memcmp(key, "name", key_len) == 0) {
 				expr_name = mp_decode_str(map, &expr_name_len);
 			} else {
-				char errmsg[DIAG_ERRMSG_MAX];
 				snprintf(errmsg, sizeof(errmsg),
 					 "invalid MsgPack map field '%.*s'",
 					 key_len, key);
@@ -322,8 +322,17 @@ checks_array_decode(const char **str, uint32_t len, char *opt, uint32_t errcode,
 				goto error;
 			}
 		}
-		sql_check_list_item_init(checks, i, expr_name, expr_name_len,
-					 expr_str, expr_str_len);
+		if (sql_check_list_item_init(checks, i, expr_name, expr_name_len,
+					     expr_str, expr_str_len) != 0) {
+			box_error_t *err = box_error_last();
+			if (box_error_code(err) != ENOMEM) {
+				snprintf(errmsg, sizeof(errmsg),
+					 "invalid expression specified");
+				diag_set(ClientError, errcode, field_no,
+					 errmsg);
+				goto error;
+			}
+		}
 	}
 	*(struct ExprList **)opt = checks;
 	return 0;
diff --git a/src/box/sql.c b/src/box/sql.c
index f129f0b..9abda39 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1797,8 +1797,11 @@ sql_check_list_item_init(struct ExprList *expr_list, int idx,
 	memset(item, 0, sizeof(*item));
 	if (expr_name != NULL) {
 		item->zName = sqlite3DbStrNDup(db, expr_name, expr_name_len);
-		if (item->zName == NULL)
+		if (item->zName == NULL) {
+			diag_set(OutOfMemory, expr_name_len, "sqlite3DbStrNDup",
+				 "item->zName");
 			return -1;
+		}
 	}
 	if (expr_str != NULL && sql_expr_compile(db, expr_str, expr_str_len,
 						 &item->pExpr) != 0) {

And some tests:
+++ b/test/sql/checks.test.lua
@@ -0,0 +1,21 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+--
+-- gh-3272: Move SQL CHECK into server
+--
+
+-- invalid expression
+opts = {checks = {{expr = 'X><5'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
+opts = {checks = {{expr = 'X>5'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+box.space._space:delete(513)
+
+test_run:cmd("clear filter")