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

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

[Kirill Shcherbatov]

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3272-no-sql-checks
Issue: https://github.com/tarantool/tarantool/issues/3272

As we need to store Checks array in opt_def MsgPack in future
introduced special type and decode callback to_array used in opt_set
function.
Removed SELECTTRACE_ENABLED macro with conditional fields from
struct Parse to prevent different object sizes across the project.
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.

Kirill Shcherbatov (7):
  sql: remove parser construct, destruct to sql.h
  box: introduce OPT_ARRAY opt_type to decode arrays
  sql: introduce expr_len for sql_expr_compile
  sql: rename sql_expr_free to sql_expr_delete
  sql: change sqlite3AddCheckConstraint signature
  sql: export funcs defined on Expr, ExprList to sql.h
  sql: remove Checks to server

 src/box/alter.cc            |   6 +++
 src/box/opt_def.c           |   9 ++++
 src/box/opt_def.h           |  20 +++++--
 src/box/space_def.c         | 100 ++++++++++++++++++++++++++++++++++-
 src/box/space_def.h         |  12 ++---
 src/box/sql.c               | 107 +++++++++++++++++++++++++++++++++++++-
 src/box/sql.h               | 123 ++++++++++++++++++++++++++++++++++++++++++-
 src/box/sql/build.c         | 124 ++++++++++++++++++++++++++------------------
 src/box/sql/delete.c        |   2 +-
 src/box/sql/expr.c          | 117 +++++++++++++++++++----------------------
 src/box/sql/fkey.c          |  21 ++++----
 src/box/sql/insert.c        |  35 +++++++------
 src/box/sql/parse.y         | 107 +++++++++++++++++++-------------------
 src/box/sql/pragma.h        |   2 -
 src/box/sql/prepare.c       |  11 +++-
 src/box/sql/resolve.c       |  51 +++++++-----------
 src/box/sql/select.c        |  58 +++++++++++----------
 src/box/sql/sqliteInt.h     |  39 +++-----------
 src/box/sql/tokenize.c      |   7 +--
 src/box/sql/trigger.c       |  20 +++----
 src/box/sql/update.c        |   4 +-
 src/box/sql/wherecode.c     |  14 ++---
 src/box/sql/whereexpr.c     |  12 ++---
 test/sql-tap/check.test.lua |  10 ++--
 24 files changed, 674 insertions(+), 337 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v7 1/7] sql: remove parser construct, destruct to sql.h

[Kirill Shcherbatov]

Moved sql_parser_construct and sql_parser_destruct to sql.h to
use them not only in DDL.
Removed SELECTTRACE_ENABLED macro with conditional fields from
struct Parse to prevent different object sizes across the project.

Part of #3272.
---
 src/box/sql.h           | 17 +++++++++++++++++
 src/box/sql/prepare.c   |  9 +++++++++
 src/box/sql/sqliteInt.h | 25 -------------------------
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/box/sql.h b/src/box/sql.h
index 3c26492..ff8ab6f 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -175,6 +175,23 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name);
 int
 sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
 
+/**
+ * Initialize a new parser object.
+ * A number of service allocations are performed on the region, which is also
+ * cleared in the destroy function.
+ * @param parser object to initialize.
+ * @param db SQLite object.
+ */
+void
+sql_parser_create(struct Parse *parser, struct sqlite3 *db);
+
+/**
+ * Release the parser object resources.
+ * @param parser object to release.
+ */
+void
+sql_parser_destroy(struct Parse *parser);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 8dc433f..8d94642 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -433,6 +433,15 @@ sqlite3_prepare_v2(sqlite3 * db,	/* Database handle. */
 }
 
 void
+sql_parser_create(struct Parse *parser, sqlite3 *db)
+{
+	memset(parser, 0, sizeof(struct Parse));
+	parser->db = db;
+	struct region *region = &fiber()->gc;
+	parser->region_initial_size = region_used(region);
+}
+
+void
 sql_parser_destroy(Parse *parser)
 {
 	assert(parser != NULL);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 99c9fea..a7ef80f 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2911,10 +2911,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
 	int nSelect;		/* Number of SELECT statements seen */
 	int nSelectIndent;	/* How far to indent SELECTTRACE() output */
-#endif
 	Parse *pToplevel;	/* Parse structure for main program (or NULL) */
 	Table *pTriggerTab;	/* Table triggers are being coded for */
 	u32 nQueryLoop;		/* Est number of iterations of a query (10*log2(N)) */
@@ -4554,27 +4552,4 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
-/**
- * Initialize a new parser object.
- * A number of service allocations are performed on the region, which is also
- * cleared in the destroy function.
- * @param parser object to initialize.
- * @param db SQLite object.
- */
-static inline void
-sql_parser_create(struct Parse *parser, sqlite3 *db)
-{
-	memset(parser, 0, sizeof(struct Parse));
-	parser->db = db;
-	struct region *region = &fiber()->gc;
-	parser->region_initial_size = region_used(region);
-}
-
-/**
- * Release the parser object resources.
- * @param parser object to release.
- */
-void
-sql_parser_destroy(struct Parse *parser);
-
 #endif				/* SQLITEINT_H */
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 1/7] sql: remove parser construct, destruct to sql.h

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> Moved sql_parser_construct and sql_parser_destruct to sql.h to
> use them not only in DDL.
> Removed SELECTTRACE_ENABLED macro with conditional fields from
> struct Parse to prevent different object sizes across the project.
> 
> Part of #3272.

LGTM.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v7 1/7] sql: remove parser construct, destruct to sql.h

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See a pair of comments below.

On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> Moved sql_parser_construct and sql_parser_destruct to sql.h to

1. I do not see such functions anywhere. Do you mean
sql_parser_create/destroy?.

> use them not only in DDL.
> Removed SELECTTRACE_ENABLED macro with conditional fields from
> struct Parse to prevent different object sizes across the project.
> 
> Part of #3272.
> ---
>   src/box/sql.h           | 17 +++++++++++++++++
>   src/box/sql/prepare.c   |  9 +++++++++
>   src/box/sql/sqliteInt.h | 25 -------------------------
>   3 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 3c26492..ff8ab6f 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -175,6 +175,23 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name);
>   int
>   sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
>   
> +/**
> + * Initialize a new parser object.
> + * A number of service allocations are performed on the region, which is also

2. Out of 66.

> + * cleared in the destroy function.
> + * @param parser object to initialize.
> + * @param db SQLite object.
> + */
> +void
> +sql_parser_create(struct Parse *parser, struct sqlite3 *db);
> +

[tarantool-patches] Re: [PATCH v7 1/7] sql: remove parser construct, destruct to sql.h

[Kirill Shcherbatov]

> 1. I do not see such functions anywhere. Do you mean
> sql_parser_create/destroy?.
> 2. Out of 66.
Fixed all in branch.

[tarantool-patches] [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Kirill Shcherbatov]

As we need to store Checks array in opt_def MsgPack in future
introduced special type and decode callback to_array used in opt_set
function.

Part of #3272.
---
 src/box/opt_def.c |  9 +++++++++
 src/box/opt_def.h | 19 ++++++++++++++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/box/opt_def.c b/src/box/opt_def.c
index cd93c23..c8440c9 100644
--- a/src/box/opt_def.c
+++ b/src/box/opt_def.c
@@ -44,6 +44,7 @@ const char *opt_type_strs[] = {
 	/* [OPT_STR]	= */ "string",
 	/* [OPT_STRPTR] = */ "string",
 	/* [OPT_ENUM]   = */ "enum",
+	/* [OPT_ARRAY]  = */ "array",
 };
 
 static int
@@ -135,6 +136,14 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 			unreachable();
 		};
 		break;
+	case OPT_ARRAY:
+		if (mp_typeof(**val) != MP_ARRAY)
+			return -1;
+		ival = mp_decode_array(val);
+		assert(def->to_array != NULL);
+		if (def->to_array(val, ival, opt) != 0)
+			return -1;
+		break;
 	default:
 		unreachable();
 	}
diff --git a/src/box/opt_def.h b/src/box/opt_def.h
index 633832a..71fef3a 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 */
 	opt_type_MAX,
 };
 
 extern const char *opt_type_strs[];
 
 typedef int64_t (*opt_def_to_enum_cb)(const char *str, uint32_t len);
+typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt);
 
 struct opt_def {
 	const char *name;
@@ -64,19 +66,26 @@ struct opt_def {
 	int enum_size;
 	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;
+	/** If not NULL, used to get a value by a string. */
+	union {
+		opt_def_to_enum_cb to_enum;
+		opt_def_to_array_cb to_array;
+	};
 };
 
 #define OPT_DEF(key, type, opts, field) \
 	{ key, type, offsetof(opts, field), sizeof(((opts *)0)->field), \
-	  NULL, 0, NULL, 0, NULL }
+	  NULL, 0, NULL, 0, {NULL} }
 
 #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} }
 
-#define OPT_END {NULL, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, NULL}
+#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} }
+
+#define OPT_END {NULL, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, {NULL}}
 
 struct region;
 
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> As we need to store Checks array in opt_def MsgPack in future
> introduced special type and decode callback to_array used in opt_set
> function.
> 
> Part of #3272.


> ---
>  src/box/opt_def.c |  9 +++++++++
>  src/box/opt_def.h | 19 ++++++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/opt_def.c b/src/box/opt_def.c
> index cd93c23..c8440c9 100644
> --- a/src/box/opt_def.c
> +++ b/src/box/opt_def.c
> @@ -44,6 +44,7 @@ const char *opt_type_strs[] = {
>  	/* [OPT_STR]	= */ "string",
>  	/* [OPT_STRPTR] = */ "string",
>  	/* [OPT_ENUM]   = */ "enum",
> +	/* [OPT_ARRAY]  = */ "array",
>  };
>  
>  static int
> @@ -135,6 +136,14 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
>  			unreachable();
>  		};
>  		break;
> +	case OPT_ARRAY:
> +		if (mp_typeof(**val) != MP_ARRAY)
> +			return -1;
> +		ival = mp_decode_array(val);
> +		assert(def->to_array != NULL);
> +		if (def->to_array(val, ival, opt) != 0)
> +			return -1;
> +		break;
>  	default:
>  		unreachable();
>  	}
> diff --git a/src/box/opt_def.h b/src/box/opt_def.h
> index 633832a..71fef3a 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 */
>  	opt_type_MAX,
>  };
>  
>  extern const char *opt_type_strs[];
>  
>  typedef int64_t (*opt_def_to_enum_cb)(const char *str, uint32_t len);
> +typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt);

While the previous callback is self-explanatory, this one requires
a comment. Actually a comment for both won't hurt. What should the
callback return? who does own the memory used by the return value? 

What is the error handling convention?

>  struct opt_def {
>  	const char *name;
> @@ -64,19 +66,26 @@ struct opt_def {
>  	int enum_size;
>  	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;
> +	/** If not NULL, used to get a value by a string. */

The comment became worse, not better.

> +	union {
> +		opt_def_to_enum_cb to_enum;
> +		opt_def_to_array_cb to_array;
> +	};
>  };

Otherwise LGTM.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Kirill Shcherbatov]

> While the previous callback is self-explanatory, this one requires
> a comment. Actually a comment for both won't hurt. What should the
> callback return? who does own the memory used by the return value? 
> What is the error handling convention?

+/**
+ * Decode enum stored in MsgPack.
+ * @param str encoded data pointer (next to MsgPack ENUM header).
+ * @param len str length.
+ * @retval string index or hmax if the string is not found.
+ */
 typedef int64_t (*opt_def_to_enum_cb)(const char *str, uint32_t len);
+
+/**
+ * Decode MsgPack array callback.
+ * All memory allocations returned by opt_def_to_array_cb with opt
+ * [out] argument should be managed manually.
+ * @param str encoded data pointer (next to MsgPack ARRAY header).
+ * @param len array length (items count).
+ * @param [out] opt pointer to store resulting value.
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
 typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt);
 > The comment became worse, not better.
- /** If not NULL, used to get a value by a string. */
+ /** MsgPack data decode callbacks. */


> Otherwise LGTM.

[tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Vladislav Shpilevoy]

Thanks for the patch! See 1 comment below.

On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> As we need to store Checks array in opt_def MsgPack in future
> introduced special type and decode callback to_array used in opt_set
> function.
> 
> Part of #3272.
> ---
>   src/box/opt_def.c |  9 +++++++++
>   src/box/opt_def.h | 19 ++++++++++++++-----
>   2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/opt_def.c b/src/box/opt_def.c
> index cd93c23..c8440c9 100644
> --- a/src/box/opt_def.c
> +++ b/src/box/opt_def.c
> @@ -44,6 +44,7 @@ const char *opt_type_strs[] = {
>   	/* [OPT_STR]	= */ "string",
>   	/* [OPT_STRPTR] = */ "string",
>   	/* [OPT_ENUM]   = */ "enum",
> +	/* [OPT_ARRAY]  = */ "array",
>   };
>   
>   static int
> @@ -135,6 +136,14 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
>   			unreachable();
>   		};
>   		break;
> +	case OPT_ARRAY:
> +		if (mp_typeof(**val) != MP_ARRAY)
> +			return -1;
> +		ival = mp_decode_array(val);
> +		assert(def->to_array != NULL);
> +		if (def->to_array(val, ival, opt) != 0)
> +			return -1;

1. If to_array is failed, the error is reset in opts_parse_key:

snprintf(errmsg, sizeof(errmsg), "'%.*s' must be %s",
	 key_len, key, opt_type_strs[def->type]);
diag_set(ClientError, errcode, field_no, errmsg);

So the original error is lost. You should refactor opt_set
so that it sets diag on type mismatch instead of doing it in
opts_parse_key. And opts_parse_key does nothing on error in opt_set -
just return -1.

> +		break;
>   	default:
>   		unreachable();
>   	}

[tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Kirill Shcherbatov]

> 1. If to_array is failed, the error is reset in opts_parse_key:
> 
> snprintf(errmsg, sizeof(errmsg), "'%.*s' must be %s",
> 	 key_len, key, opt_type_strs[def->type]);
> diag_set(ClientError, errcode, field_no, errmsg);
> 
> So the original error is lost. You should refactor opt_set
> so that it sets diag on type mismatch instead of doing it in
> opts_parse_key. And opts_parse_key does nothing on error in opt_set -
> just return -1.

diff --git a/src/box/opt_def.c b/src/box/opt_def.c
index c8440c9..f4dd05b 100644
--- a/src/box/opt_def.c
+++ b/src/box/opt_def.c
@@ -49,10 +49,11 @@ const char *opt_type_strs[] = {
 
 static int
 opt_set(void *opts, const struct opt_def *def, const char **val,
-	struct region *region)
+	struct region *region, uint32_t errcode, uint32_t field_no)
 {
 	int64_t ival;
 	uint64_t uval;
+	char errmsg[DIAG_ERRMSG_MAX];
 	double dval;
 	uint32_t str_len;
 	const char *str;
@@ -61,30 +62,30 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 	switch (def->type) {
 	case OPT_BOOL:
 		if (mp_typeof(**val) != MP_BOOL)
-			return -1;
+			goto type_mismatch_err;
 		store_bool(opt, mp_decode_bool(val));
 		break;
 	case OPT_UINT32:
 		if (mp_typeof(**val) != MP_UINT)
-			return -1;
+			goto type_mismatch_err;
 		uval = mp_decode_uint(val);
 		if (uval > UINT32_MAX)
-			return -1;
+			goto type_mismatch_err;
 		store_u32(opt, uval);
 		break;
 	case OPT_INT64:
 		if (mp_read_int64(val, &ival) != 0)
-			return -1;
+			goto type_mismatch_err;
 		store_u64(opt, ival);
 		break;
 	case OPT_FLOAT:
 		if (mp_read_double(val, &dval) != 0)
-			return -1;
+			goto type_mismatch_err;
 		store_double(opt, dval);
 		break;
 	case OPT_STR:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		str_len = MIN(str_len, def->len - 1);
 		memcpy(opt, str, str_len);
@@ -92,7 +93,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_STRPTR:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		if (str_len > 0) {
 			ptr = (char *) region_alloc(region, str_len + 1);
@@ -111,7 +112,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_ENUM:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		if (def->to_enum == NULL) {
 			ival = strnindex(def->enum_strs, str, str_len,
@@ -138,7 +139,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_ARRAY:
 		if (mp_typeof(**val) != MP_ARRAY)
-			return -1;
+			goto type_mismatch_err;
 		ival = mp_decode_array(val);
 		assert(def->to_array != NULL);
 		if (def->to_array(val, ival, opt) != 0)
@@ -148,6 +149,12 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		unreachable();
 	}
 	return 0;
+
+type_mismatch_err:
+	snprintf(errmsg, sizeof(errmsg), "'%s' must be %s",
+		 def->name, opt_type_strs[def->type]);
+	diag_set(ClientError, errcode, field_no, errmsg);
+	return -1;
 }
 
 int
@@ -163,12 +170,8 @@ opts_parse_key(void *opts, const struct opt_def *reg, const char *key,
 		    memcmp(key, def->name, key_len) != 0)
 			continue;
 
-		if (opt_set(opts, def, data, region) != 0) {
-			snprintf(errmsg, sizeof(errmsg), "'%.*s' must be %s",
-				 key_len, key, opt_type_strs[def->type]);
-			diag_set(ClientError, errcode, field_no, errmsg);
+		if (opt_set(opts, def, data, region, errcode, field_no) != 0)
 			return -1;
-		}
 		found = true;
 		break;
 	}

And some other changes

+/**
+ * Decode MsgPack array callback.
+ * All memory allocations returned by opt_def_to_array_cb with opt
+ * [out] argument should be managed manually.
+ * @param str encoded data pointer (next to MsgPack ARRAY header).
+ * @param len array length (items count).
+ * @param [out] opt pointer to store resulting value.
+ * @param errcode Code of error to set if something is wrong.
+ * @param field_no Field number of an option in a parent element.
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt,
+                                  uint32_t errcode, uint32_t field_no);


+       case OPT_ARRAY:
+               if (mp_typeof(**val) != MP_ARRAY)
+                       goto type_mismatch_err;
+               ival = mp_decode_array(val);
+               assert(def->to_array != NULL);
+               if (def->to_array(val, ival, opt, errcode, field_no) != 0)
+                       return -1;
+               break;

[tarantool-patches] [PATCH v7 3/7] sql: introduce expr_len for sql_expr_compile

[Kirill Shcherbatov]

As we need to use parser in the cases with not null-terminated
strings expr_len argument for sql_expr_compile is required.

Part of #3272.
---
 src/box/alter.cc       | 1 +
 src/box/sql.h          | 4 +++-
 src/box/sql/tokenize.c | 7 ++++---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index e87bbce..e4780da 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -408,6 +408,7 @@ field_def_decode(struct field_def *field, const char **data,
 
 	if (field->default_value != NULL &&
 	    sql_expr_compile(sql_get(), field->default_value,
+			     strlen(field->default_value),
 			     &field->default_value_expr) != 0)
 		diag_raise();
 }
diff --git a/src/box/sql.h b/src/box/sql.h
index ff8ab6f..a7f2500 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.
  * @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);
 
 /**
  * Store duplicate of a parsed expression into @a parser.
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 9146e95..04e9801 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -649,16 +649,17 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 }
 
 int
-sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
+sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
+		 struct Expr **result)
 {
 	const char *outer = "SELECT ";
-	int len = strlen(outer) + strlen(expr);
+	int len = strlen(outer) + expr_len;
 	char *stmt = (char *) region_alloc(&fiber()->gc, len + 1);
 	if (stmt == NULL) {
 		diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
 		return -1;
 	}
-	sprintf(stmt, "%s%s", outer, expr);
+	sprintf(stmt, "%s%.*s", outer, expr_len, expr);
 
 	struct Parse parser;
 	sql_parser_create(&parser, db);
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 3/7] sql: introduce expr_len for sql_expr_compile

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments below.

On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> As we need to use parser in the cases with not null-terminated
> strings expr_len argument for sql_expr_compile is required.
> 
> Part of #3272.
> ---
>   src/box/alter.cc       | 1 +
>   src/box/sql.h          | 4 +++-
>   src/box/sql/tokenize.c | 7 ++++---
>   3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index e87bbce..e4780da 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -408,6 +408,7 @@ field_def_decode(struct field_def *field, const char **data,
>   
>   	if (field->default_value != NULL &&
>   	    sql_expr_compile(sql_get(), field->default_value,
> +			     strlen(field->default_value),
>   			     &field->default_value_expr) != 0)
>   		diag_raise();
>   }
> diff --git a/src/box/sql.h b/src/box/sql.h
> index ff8ab6f..a7f2500 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.

1. Looks like expr_len is the expression that is intended to
parse a length.
Do you mean this?: "Length of @an expr."

>    * @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);

2. Bad indentation.

[tarantool-patches] Re: [PATCH v7 3/7] sql: introduce expr_len for sql_expr_compile

[Kirill Shcherbatov]

> 1. Looks like expr_len is the expression that is intended to
> parse a length.
> Do you mean this?: "Length of @an expr."

- * @param expr_len Expression to parse length.
+ * @param expr_len Length of @an expr..

> 2. Bad indentation.
 int
sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
- struct Expr **result);
+ struct Expr **result);

[tarantool-patches] [PATCH v7 4/7] sql: rename sql_expr_free to sql_expr_delete

[Kirill Shcherbatov]

According our convention resource release functions should
have _delete but not _free suffix.

Part of #3272.
---
 src/box/space_def.c     |  4 ++--
 src/box/sql.h           |  2 +-
 src/box/sql/build.c     | 12 ++++++------
 src/box/sql/delete.c    |  2 +-
 src/box/sql/expr.c      | 26 +++++++++++++-------------
 src/box/sql/fkey.c      | 10 +++++-----
 src/box/sql/parse.y     | 22 +++++++++++-----------
 src/box/sql/resolve.c   | 12 ++++++------
 src/box/sql/select.c    | 18 +++++++++---------
 src/box/sql/trigger.c   | 12 ++++++------
 src/box/sql/update.c    |  2 +-
 src/box/sql/wherecode.c |  2 +-
 src/box/sql/whereexpr.c |  6 +++---
 13 files changed, 65 insertions(+), 65 deletions(-)

diff --git a/src/box/space_def.c b/src/box/space_def.c
index 2f0a9f0..9e0e7e3 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -216,8 +216,8 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
 {
 	for (uint32_t i = 0; i < field_count; ++i) {
 		if (fields[i].default_value_expr != NULL) {
-			sql_expr_free(sql_get(), fields[i].default_value_expr,
-				      true);
+			sql_expr_delete(sql_get(), fields[i].default_value_expr,
+					true);
 		}
 	}
 }
diff --git a/src/box/sql.h b/src/box/sql.h
index a7f2500..20f0651 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -144,7 +144,7 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
  * @param extern_alloc True if skeleton was allocated externally.
  */
 void
-sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
+sql_expr_delete(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
 
 /**
  * Create and initialize a new ephemeral SQL Table object.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e376295..a6ddcf0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -251,7 +251,7 @@ sqlite3LocateIndex(sqlite3 * db, const char *zName, const char *zTable)
 static void
 freeIndex(sqlite3 * db, Index * p)
 {
-	sql_expr_free(db, p->pPartIdxWhere, false);
+	sql_expr_delete(db, p->pPartIdxWhere, false);
 	sqlite3ExprListDelete(db, p->aColExpr);
 	sqlite3DbFree(db, p->zColAff);
 	sqlite3DbFree(db, p);
@@ -923,7 +923,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
 			field->default_value[default_length] = '\0';
 		}
 	}
-	sql_expr_free(db, pSpan->pExpr, false);
+	sql_expr_delete(db, pSpan->pExpr, false);
 }
 
 
@@ -1038,7 +1038,7 @@ sqlite3AddCheckConstraint(Parse * pParse,	/* Parsing context */
 		}
 	} else
 #endif
-		sql_expr_free(pParse->db, pCheckExpr, false);
+		sql_expr_delete(pParse->db, pCheckExpr, false);
 }
 
 /*
@@ -3239,7 +3239,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
  exit_create_index:
 	if (pIndex)
 		freeIndex(db, pIndex);
-	sql_expr_free(db, where, false);
+	sql_expr_delete(db, where, false);
 	sqlite3ExprListDelete(db, col_list);
 	sqlite3SrcListDelete(db, tbl_name);
 	sqlite3DbFree(db, zName);
@@ -3655,7 +3655,7 @@ sqlite3SrcListDelete(sqlite3 * db, SrcList * pList)
 			sqlite3ExprListDelete(db, pItem->u1.pFuncArg);
 		sqlite3DeleteTable(db, pItem->pTab);
 		sqlite3SelectDelete(db, pItem->pSelect);
-		sql_expr_free(db, pItem->pOn, false);
+		sql_expr_delete(db, pItem->pOn, false);
 		sqlite3IdListDelete(db, pItem->pUsing);
 	}
 	sqlite3DbFree(db, pList);
@@ -3711,7 +3711,7 @@ sqlite3SrcListAppendFromTerm(Parse * pParse,	/* Parsing context */
 
  append_from_error:
 	assert(p == 0);
-	sql_expr_free(db, pOn, false);
+	sql_expr_delete(db, pOn, false);
 	sqlite3IdListDelete(db, pUsing);
 	sqlite3SelectDelete(db, pSubquery);
 	return 0;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5572763..ddad54b 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -391,7 +391,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 
  delete_from_cleanup:
 	sqlite3SrcListDelete(db, tab_list);
-	sql_expr_free(db, where, false);
+	sql_expr_delete(db, where, false);
 }
 
 void
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8259680..64991ee 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -464,7 +464,7 @@ sqlite3ExprForVectorField(Parse * pParse,	/* Parsing context */
 		 * pLeft->iTable:   First in an array of register holding result, or 0
 		 *                  if the result is not yet computed.
 		 *
-		 * sql_expr_free() specifically skips the recursive delete of
+		 * sql_expr_delete() specifically skips the recursive delete of
 		 * pLeft on TK_SELECT_COLUMN nodes.  But pRight is followed, so pVector
 		 * can be attached to pRight to cause this node to take ownership of
 		 * pVector.  Typically there will be multiple TK_SELECT_COLUMN nodes
@@ -903,8 +903,8 @@ sqlite3ExprAttachSubtrees(sqlite3 * db,
 {
 	if (pRoot == 0) {
 		assert(db->mallocFailed);
-		sql_expr_free(db, pLeft, false);
-		sql_expr_free(db, pRight, false);
+		sql_expr_delete(db, pLeft, false);
+		sql_expr_delete(db, pRight, false);
 	} else {
 		if (pRight) {
 			pRoot->pRight = pRight;
@@ -1020,8 +1020,8 @@ sqlite3ExprAnd(sqlite3 * db, Expr * pLeft, Expr * pRight)
 	} else if (pRight == 0) {
 		return pLeft;
 	} else if (exprAlwaysFalse(pLeft) || exprAlwaysFalse(pRight)) {
-		sql_expr_free(db, pLeft, false);
-		sql_expr_free(db, pRight, false);
+		sql_expr_delete(db, pLeft, false);
+		sql_expr_delete(db, pRight, false);
 		return sqlite3ExprAlloc(db, TK_INTEGER, &sqlite3IntTokens[0],
 					0);
 	} else {
@@ -1158,7 +1158,7 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p, bool extern_alloc)
 		if (p->pLeft && p->op != TK_SELECT_COLUMN && !extern_alloc)
 			sqlite3ExprDeleteNN(db, p->pLeft, extern_alloc);
 		if (!extern_alloc)
-			sql_expr_free(db, p->pRight, extern_alloc);
+			sql_expr_delete(db, p->pRight, extern_alloc);
 		if (ExprHasProperty(p, EP_xIsSelect)) {
 			sqlite3SelectDelete(db, p->x.pSelect);
 		} else {
@@ -1173,7 +1173,7 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p, bool extern_alloc)
 }
 
 void
-sql_expr_free(sqlite3 *db, Expr *expr, bool extern_alloc)
+sql_expr_delete(sqlite3 *db, Expr *expr, bool extern_alloc)
 {
 	if (expr != NULL)
 		sqlite3ExprDeleteNN(db, expr, extern_alloc);
@@ -1661,7 +1661,7 @@ sqlite3ExprListAppend(Parse * pParse,	/* Parsing context */
 
  no_mem:
 	/* Avoid leaking memory if malloc has failed. */
-	sql_expr_free(db, pExpr, false);
+	sql_expr_delete(db, pExpr, false);
 	sqlite3ExprListDelete(db, pList);
 	return 0;
 }
@@ -1737,7 +1737,7 @@ sqlite3ExprListAppendVector(Parse * pParse,	/* Parsing context */
 	}
 
  vector_append_error:
-	sql_expr_free(db, pExpr, false);
+	sql_expr_delete(db, pExpr, false);
 	sqlite3IdListDelete(db, pColumns);
 	return pList;
 }
@@ -1838,7 +1838,7 @@ exprListDeleteNN(sqlite3 * db, ExprList * pList)
 	struct ExprList_item *pItem;
 	assert(pList->a != 0 || pList->nExpr == 0);
 	for (pItem = pList->a, i = 0; i < pList->nExpr; i++, pItem++) {
-		sql_expr_free(db, pItem->pExpr, false);
+		sql_expr_delete(db, pItem->pExpr, false);
 		sqlite3DbFree(db, pItem->zName);
 		sqlite3DbFree(db, pItem->zSpan);
 	}
@@ -2913,7 +2913,7 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
 						  dest.iSDParm);
 				VdbeComment((v, "Init EXISTS result"));
 			}
-			sql_expr_free(pParse->db, pSel->pLimit, false);
+		sql_expr_delete(pParse->db, pSel->pLimit, false);
 			pSel->pLimit = sqlite3ExprAlloc(pParse->db, TK_INTEGER,
 							&sqlite3IntTokens[1],
 							0);
@@ -4488,7 +4488,7 @@ sqlite3ExprCodeCopy(Parse * pParse, Expr * pExpr, int target)
 	pExpr = sqlite3ExprDup(db, pExpr, 0);
 	if (!db->mallocFailed)
 		sqlite3ExprCode(pParse, pExpr, target);
-	sql_expr_free(db, pExpr, false);
+	sql_expr_delete(db, pExpr, false);
 }
 
 /*
@@ -5044,7 +5044,7 @@ sqlite3ExprIfFalseDup(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
 	if (db->mallocFailed == 0) {
 		sqlite3ExprIfFalse(pParse, pCopy, dest, jumpIfNull);
 	}
-	sql_expr_free(db, pCopy, false);
+	sql_expr_delete(db, pCopy, false);
 }
 
 /*
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index f4f7237..2ab8fdd 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -703,7 +703,7 @@ fkScanChildren(Parse * pParse,	/* Parse context */
 	}
 
 	/* Clean up the WHERE clause constructed above. */
-	sql_expr_free(db, pWhere, false);
+	sql_expr_delete(db, pWhere, false);
 	if (iFkIfZero)
 		sqlite3VdbeJumpHere(v, iFkIfZero);
 }
@@ -742,10 +742,10 @@ fkTriggerDelete(sqlite3 * dbMem, Trigger * p)
 {
 	if (p) {
 		TriggerStep *pStep = p->step_list;
-		sql_expr_free(dbMem, pStep->pWhere, false);
+		sql_expr_delete(dbMem, pStep->pWhere, false);
 		sqlite3ExprListDelete(dbMem, pStep->pExprList);
 		sqlite3SelectDelete(dbMem, pStep->pSelect);
-		sql_expr_free(dbMem, p->pWhen, false);
+		sql_expr_delete(dbMem, p->pWhen, false);
 		sqlite3DbFree(dbMem, p);
 	}
 }
@@ -1414,8 +1414,8 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 		/* Re-enable the lookaside buffer, if it was disabled earlier. */
 		db->lookaside.bDisable--;
 
-		sql_expr_free(db, pWhere, false);
-		sql_expr_free(db, pWhen, false);
+		sql_expr_delete(db, pWhere, false);
+		sql_expr_delete(db, pWhen, false);
 		sqlite3ExprListDelete(db, pList);
 		sqlite3SelectDelete(db, pSelect);
 		if (db->mallocFailed == 1) {
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 8059889..8eac310 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -635,7 +635,7 @@ joinop(X) ::= JOIN_KW(A) join_nm(B) join_nm(C) JOIN.
                   {X = sqlite3JoinType(pParse,&A,&B,&C);/*X-overwrites-A*/}
 
 %type on_opt {Expr*}
-%destructor on_opt {sql_expr_free(pParse->db, $$, false);}
+%destructor on_opt {sql_expr_delete(pParse->db, $$, false);}
 on_opt(N) ::= ON expr(E).   {N = E.pExpr;}
 on_opt(N) ::= .             {N = 0;}
 
@@ -693,7 +693,7 @@ groupby_opt(A) ::= .                      {A = 0;}
 groupby_opt(A) ::= GROUP BY nexprlist(X). {A = X;}
 
 %type having_opt {Expr*}
-%destructor having_opt {sql_expr_free(pParse->db, $$, false);}
+%destructor having_opt {sql_expr_delete(pParse->db, $$, false);}
 having_opt(A) ::= .                {A = 0;}
 having_opt(A) ::= HAVING expr(X).  {A = X.pExpr;}
 
@@ -729,7 +729,7 @@ cmd ::= with(C) DELETE FROM fullname(X) indexed_opt(I) where_opt(W). {
 }
 
 %type where_opt {Expr*}
-%destructor where_opt {sql_expr_free(pParse->db, $$, false);}
+%destructor where_opt {sql_expr_delete(pParse->db, $$, false);}
 
 where_opt(A) ::= .                    {A = 0;}
 where_opt(A) ::= WHERE expr(X).       {A = X.pExpr;}
@@ -805,9 +805,9 @@ idlist(A) ::= nm(Y).
 //
 
 %type expr {ExprSpan}
-%destructor expr {sql_expr_free(pParse->db, $$.pExpr, false);}
+%destructor expr {sql_expr_delete(pParse->db, $$.pExpr, false);}
 %type term {ExprSpan}
-%destructor term {sql_expr_free(pParse->db, $$.pExpr, false);}
+%destructor term {sql_expr_delete(pParse->db, $$.pExpr, false);}
 
 %include {
   /* This is a utility routine used to set the ExprSpan.zStart and
@@ -1015,7 +1015,7 @@ expr(A) ::= expr(A) NOT NULL(E). {spanUnaryPostfix(pParse,TK_NOTNULL,&A,&E);}
     sqlite3 *db = pParse->db;
     if( pA && pY && pY->op==TK_NULL ){
       pA->op = (u8)op;
-      sql_expr_free(db, pA->pRight, false);
+      sql_expr_delete(db, pA->pRight, false);
       pA->pRight = 0;
     }
   }
@@ -1091,7 +1091,7 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
     ** simplify to constants 0 (false) and 1 (true), respectively,
     ** regardless of the value of expr1.
     */
-    sql_expr_free(pParse->db, A.pExpr, false);
+    sql_expr_delete(pParse->db, A.pExpr, false);
     A.pExpr = sqlite3ExprAlloc(pParse->db, TK_INTEGER,&sqlite3IntTokens[N],1);
   }else if( Y->nExpr==1 ){
     /* Expressions of the form:
@@ -1168,7 +1168,7 @@ expr(A) ::= CASE(C) case_operand(X) case_exprlist(Y) case_else(Z) END(E). {
     sqlite3ExprSetHeightAndFlags(pParse, A.pExpr);
   }else{
     sqlite3ExprListDelete(pParse->db, Y);
-    sql_expr_free(pParse->db, Z, false);
+    sql_expr_delete(pParse->db, Z, false);
   }
 }
 %type case_exprlist {ExprList*}
@@ -1182,11 +1182,11 @@ case_exprlist(A) ::= WHEN expr(Y) THEN expr(Z). {
   A = sqlite3ExprListAppend(pParse,A, Z.pExpr);
 }
 %type case_else {Expr*}
-%destructor case_else {sql_expr_free(pParse->db, $$, false);}
+%destructor case_else {sql_expr_delete(pParse->db, $$, false);}
 case_else(A) ::=  ELSE expr(X).         {A = X.pExpr;}
 case_else(A) ::=  .                     {A = 0;} 
 %type case_operand {Expr*}
-%destructor case_operand {sql_expr_free(pParse->db, $$, false);}
+%destructor case_operand {sql_expr_delete(pParse->db, $$, false);}
 case_operand(A) ::= expr(X).            {A = X.pExpr; /*A-overwrites-X*/} 
 case_operand(A) ::= .                   {A = 0;} 
 
@@ -1353,7 +1353,7 @@ foreach_clause ::= .
 foreach_clause ::= FOR EACH ROW.
 
 %type when_clause {Expr*}
-%destructor when_clause {sql_expr_free(pParse->db, $$, false);}
+%destructor when_clause {sql_expr_delete(pParse->db, $$, false);}
 when_clause(A) ::= .             { A = 0; }
 when_clause(A) ::= WHEN expr(X). { A = X.pExpr; }
 
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 58aa980..7f0858a 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -115,7 +115,7 @@ resolveAlias(Parse * pParse,	/* Parsing context */
 	}
 	ExprSetProperty(pDup, EP_Alias);
 
-	/* Before calling sql_expr_free(), set the EP_Static flag. This
+	/* Before calling sql_expr_delete(), set the EP_Static flag. This
 	 * prevents ExprDelete() from deleting the Expr structure itself,
 	 * allowing it to be repopulated by the memcpy() on the following line.
 	 * The pExpr->u.zToken might point into memory that will be freed by the
@@ -123,7 +123,7 @@ resolveAlias(Parse * pParse,	/* Parsing context */
 	 * make a copy of the token before doing the sqlite3DbFree().
 	 */
 	ExprSetProperty(pExpr, EP_Static);
-	sql_expr_free(db, pExpr, false);
+	sql_expr_delete(db, pExpr, false);
 	memcpy(pExpr, pDup, sizeof(*pExpr));
 	if (!ExprHasProperty(pExpr, EP_IntValue) && pExpr->u.zToken != 0) {
 		assert((pExpr->flags & (EP_Reduced | EP_TokenOnly)) == 0);
@@ -464,9 +464,9 @@ lookupName(Parse * pParse,	/* The parsing context */
 
 	/* Clean up and return
 	 */
-	sql_expr_free(db, pExpr->pLeft, false);
+	sql_expr_delete(db, pExpr->pLeft, false);
 	pExpr->pLeft = 0;
-	sql_expr_free(db, pExpr->pRight, false);
+	sql_expr_delete(db, pExpr->pRight, false);
 	pExpr->pRight = 0;
 	pExpr->op = (isTrigger ? TK_TRIGGER : TK_COLUMN);
  lookupname_end:
@@ -1029,7 +1029,7 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
 						    resolveOrderByTermToExprList
 						    (pParse, pSelect, pDup);
 					}
-					sql_expr_free(db, pDup, false);
+					sql_expr_delete(db, pDup, false);
 				}
 			}
 			if (iCol > 0) {
@@ -1051,7 +1051,7 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
 					assert(pParent->pLeft == pE);
 					pParent->pLeft = pNew;
 				}
-				sql_expr_free(db, pE, false);
+				sql_expr_delete(db, pE, false);
 				pItem->u.x.iOrderByCol = (u16) iCol;
 				pItem->done = 1;
 			} else {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 5fbcbaf..75bd53d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -97,12 +97,12 @@ clearSelect(sqlite3 * db, Select * p, int bFree)
 		Select *pPrior = p->pPrior;
 		sqlite3ExprListDelete(db, p->pEList);
 		sqlite3SrcListDelete(db, p->pSrc);
-		sql_expr_free(db, p->pWhere, false);
+		sql_expr_delete(db, p->pWhere, false);
 		sqlite3ExprListDelete(db, p->pGroupBy);
-		sql_expr_free(db, p->pHaving, false);
+		sql_expr_delete(db, p->pHaving, false);
 		sqlite3ExprListDelete(db, p->pOrderBy);
-		sql_expr_free(db, p->pLimit, false);
-		sql_expr_free(db, p->pOffset, false);
+		sql_expr_delete(db, p->pLimit, false);
+		sql_expr_delete(db, p->pOffset, false);
 		if (p->pWith)
 			sqlite3WithDelete(db, p->pWith);
 		if (bFree)
@@ -2639,7 +2639,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
 							     pPrior->
 							     nSelectRow);
 				}
-				sql_expr_free(db, p->pLimit, false);
+			sql_expr_delete(db, p->pLimit, false);
 				p->pLimit = pLimit;
 				p->pOffset = pOffset;
 				p->iLimit = 0;
@@ -2738,7 +2738,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
 				p->pPrior = pPrior;
 				if (p->nSelectRow > pPrior->nSelectRow)
 					p->nSelectRow = pPrior->nSelectRow;
-				sql_expr_free(db, p->pLimit, false);
+			sql_expr_delete(db, p->pLimit, false);
 				p->pLimit = pLimit;
 				p->pOffset = pOffset;
 
@@ -3277,9 +3277,9 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
 	} else {
 		regLimitA = regLimitB = 0;
 	}
-	sql_expr_free(db, p->pLimit, false);
+	sql_expr_delete(db, p->pLimit, false);
 	p->pLimit = 0;
-	sql_expr_free(db, p->pOffset, false);
+	sql_expr_delete(db, p->pOffset, false);
 	p->pOffset = 0;
 
 	regAddrA = ++pParse->nMem;
@@ -3494,7 +3494,7 @@ substExpr(Parse * pParse,	/* Report errors here */
 					    pExpr->iRightJoinTable;
 					pNew->flags |= EP_FromJoin;
 				}
-				sql_expr_free(db, pExpr, false);
+				sql_expr_delete(db, pExpr, false);
 				pExpr = pNew;
 			}
 		}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 665f65d..2704816 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -52,7 +52,7 @@ sqlite3DeleteTriggerStep(sqlite3 * db, TriggerStep * pTriggerStep)
 		TriggerStep *pTmp = pTriggerStep;
 		pTriggerStep = pTriggerStep->pNext;
 
-		sql_expr_free(db, pTmp->pWhere, false);
+		sql_expr_delete(db, pTmp->pWhere, false);
 		sqlite3ExprListDelete(db, pTmp->pExprList);
 		sqlite3SelectDelete(db, pTmp->pSelect);
 		sqlite3IdListDelete(db, pTmp->pIdList);
@@ -184,7 +184,7 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	sqlite3DbFree(db, zName);
 	sqlite3SrcListDelete(db, pTableName);
 	sqlite3IdListDelete(db, pColumns);
-	sql_expr_free(db, pWhen, false);
+	sql_expr_delete(db, pWhen, false);
 	if (!pParse->pNewTrigger) {
 		sqlite3DeleteTrigger(db, pTrigger);
 	} else {
@@ -444,7 +444,7 @@ sqlite3TriggerUpdateStep(sqlite3 * db,	/* The database connection */
 		pTriggerStep->orconf = orconf;
 	}
 	sqlite3ExprListDelete(db, pEList);
-	sql_expr_free(db, pWhere, false);
+	sql_expr_delete(db, pWhere, false);
 	return pTriggerStep;
 }
 
@@ -467,7 +467,7 @@ sqlite3TriggerDeleteStep(sqlite3 * db,	/* Database connection */
 		    sqlite3ExprDup(db, pWhere, EXPRDUP_REDUCE);
 		pTriggerStep->orconf = ON_CONFLICT_ACTION_DEFAULT;
 	}
-	sql_expr_free(db, pWhere, false);
+	sql_expr_delete(db, pWhere, false);
 	return pTriggerStep;
 }
 
@@ -482,7 +482,7 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger)
 	sqlite3DeleteTriggerStep(db, pTrigger->step_list);
 	sqlite3DbFree(db, pTrigger->zName);
 	sqlite3DbFree(db, pTrigger->table);
-	sql_expr_free(db, pTrigger->pWhen, false);
+	sql_expr_delete(db, pTrigger->pWhen, false);
 	sqlite3IdListDelete(db, pTrigger->pColumns);
 	sqlite3DbFree(db, pTrigger);
 }
@@ -903,7 +903,7 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 						   iEndTrigger,
 						   SQLITE_JUMPIFNULL);
 			}
-			sql_expr_free(db, pWhen, false);
+			sql_expr_delete(db, pWhen, false);
 		}
 
 		/* Code the trigger program into the sub-vdbe. */
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 2477c83..848ae20 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -643,6 +643,6 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	sqlite3DbFree(db, aXRef);	/* Also frees aRegIdx[] and aToOpen[] */
 	sqlite3SrcListDelete(db, pTabList);
 	sqlite3ExprListDelete(db, pChanges);
-	sql_expr_free(db, pWhere, false);
+	sql_expr_delete(db, pWhere, false);
 	return;
 }
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 9685af0..8b0f21a 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1818,7 +1818,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 			pLevel->iIdxCur = iCovCur;
 		if (pAndExpr) {
 			pAndExpr->pLeft = 0;
-			sql_expr_free(db, pAndExpr, false);
+			sql_expr_delete(db, pAndExpr, false);
 		}
 		sqlite3VdbeChangeP1(v, iRetInit, sqlite3VdbeCurrentAddr(v));
 		sqlite3VdbeGoto(v, pLevel->addrBrk);
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 26c1b69..6da2762 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -97,7 +97,7 @@ whereClauseInsert(WhereClause * pWC, Expr * p, u16 wtFlags)
 					 sizeof(pWC->a[0]) * pWC->nSlot * 2);
 		if (pWC->a == 0) {
 			if (wtFlags & TERM_DYNAMIC) {
-				sql_expr_free(db, p, false);
+				sql_expr_delete(db, p, false);
 			}
 			pWC->a = pOld;
 			return 0;
@@ -1066,7 +1066,7 @@ exprAnalyze(SrcList * pSrc,	/* the FROM clause */
 				int idxNew;
 				pDup = sqlite3ExprDup(db, pExpr, 0);
 				if (db->mallocFailed) {
-					sql_expr_free(db, pDup, false);
+					sql_expr_delete(db, pDup, false);
 					return;
 				}
 				idxNew =
@@ -1405,7 +1405,7 @@ sqlite3WhereClauseClear(WhereClause * pWC)
 	sqlite3 *db = pWC->pWInfo->pParse->db;
 	for (i = pWC->nTerm - 1, a = pWC->a; i >= 0; i--, a++) {
 		if (a->wtFlags & TERM_DYNAMIC) {
-			sql_expr_free(db, a->pExpr, false);
+			sql_expr_delete(db, a->pExpr, false);
 		}
 		if (a->wtFlags & TERM_ORINFO) {
 			whereOrInfoDelete(db, a->u.pOrInfo);
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 4/7] sql: rename sql_expr_free to sql_expr_delete

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> According our convention resource release functions should
> have _delete but not _free suffix.

OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v7 4/7] sql: rename sql_expr_free to sql_expr_delete

[Vladislav Shpilevoy]

Thanks for the patch! See 3 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 8259680..64991ee 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -2913,7 +2913,7 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>   						  dest.iSDParm);
>   				VdbeComment((v, "Init EXISTS result"));
>   			}
> -			sql_expr_free(pParse->db, pSel->pLimit, false);
> +		sql_expr_delete(pParse->db, pSel->pLimit, false);

1. Bad indentation.

>   			pSel->pLimit = sqlite3ExprAlloc(pParse->db, TK_INTEGER,
>   							&sqlite3IntTokens[1],
>   							0);
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5fbcbaf..75bd53d 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2639,7 +2639,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   							     pPrior->
>   							     nSelectRow);
>   				}
> -				sql_expr_free(db, p->pLimit, false);
> +			sql_expr_delete(db, p->pLimit, false);

2. Same.

>   				p->pLimit = pLimit;
>   				p->pOffset = pOffset;
>   				p->iLimit = 0;
> @@ -2738,7 +2738,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   				p->pPrior = pPrior;
>   				if (p->nSelectRow > pPrior->nSelectRow)
>   					p->nSelectRow = pPrior->nSelectRow;
> -				sql_expr_free(db, p->pLimit, false);
> +			sql_expr_delete(db, p->pLimit, false);

3. Same.

>   				p->pLimit = pLimit;
>   				p->pOffset = pOffset;
>   

[tarantool-patches] Re: [PATCH v7 4/7] sql: rename sql_expr_free to sql_expr_delete

[Kirill Shcherbatov]

> 1. Bad indentation.
-               sql_expr_delete(pParse->db, pSel->pLimit, false);
+                       sql_expr_delete(pParse->db, pSel->pLimit, false);

> 2. Same.
-                       sql_expr_delete(db, p->pLimit, false);
+                               sql_expr_delete(db, p->pLimit, false);

> 3. Same.
-                       sql_expr_delete(db, p->pLimit, false);
+                               sql_expr_delete(db, p->pLimit, false);

[tarantool-patches] [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Kirill Shcherbatov]

As we would store Check source expr in MsgPack we need
span instead of parsed Expr only.
Renamed refactored function to sql_add_check_constraint.

Part of #3272.
---
 src/box/sql/build.c     | 24 ++++++++++--------------
 src/box/sql/parse.y     |  4 ++--
 src/box/sql/sqliteInt.h |  9 ++++++++-
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a6ddcf0..afb1128 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1019,26 +1019,22 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 	return;
 }
 
-/*
- * Add a new CHECK constraint to the table currently under construction.
- */
 void
-sqlite3AddCheckConstraint(Parse * pParse,	/* Parsing context */
-			  Expr * pCheckExpr	/* The check expression */
-    )
+sql_add_check_constraint(Parse *parser, ExprSpan *span)
 {
 #ifndef SQLITE_OMIT_CHECK
-	Table *pTab = pParse->pNewTable;
-	if (pTab) {
-		pTab->pCheck =
-		    sqlite3ExprListAppend(pParse, pTab->pCheck, pCheckExpr);
-		if (pParse->constraintName.n) {
-			sqlite3ExprListSetName(pParse, pTab->pCheck,
-					       &pParse->constraintName, 1);
+	struct Expr *expr = span->pExpr;
+	Table *table = parser->pNewTable;
+	if (table != NULL) {
+		table->pCheck =
+			sqlite3ExprListAppend(parser, table->pCheck, expr);
+		if (parser->constraintName.n) {
+			sqlite3ExprListSetName(parser, table->pCheck,
+					       &parser->constraintName, 1);
 		}
 	} else
 #endif
-		sql_expr_delete(pParse->db, pCheckExpr, false);
+		sql_expr_delete(parser->db, expr, false);
 }
 
 /*
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 8eac310..cc3931d 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -284,7 +284,7 @@ ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I).
 ccons ::= UNIQUE onconf(R).      {sql_create_index(pParse,0,0,0,R,0,0,
 						   SORT_ORDER_ASC, false,
 						   SQLITE_IDXTYPE_UNIQUE);}
-ccons ::= CHECK LP expr(X) RP.   {sqlite3AddCheckConstraint(pParse,X.pExpr);}
+ccons ::= CHECK LP expr(X) RP.   {sql_add_check_constraint(pParse,&X);}
 ccons ::= REFERENCES nm(T) eidlist_opt(TA) refargs(R).
                                  {sqlite3CreateForeignKey(pParse,0,&T,TA,R);}
 ccons ::= defer_subclause(D).    {sqlite3DeferForeignKey(pParse,D);}
@@ -336,7 +336,7 @@ tcons ::= UNIQUE LP sortlist(X) RP onconf(R).
 						   SORT_ORDER_ASC,false,
 						   SQLITE_IDXTYPE_UNIQUE);}
 tcons ::= CHECK LP expr(E) RP onconf.
-                                 {sqlite3AddCheckConstraint(pParse,E.pExpr);}
+                                 {sql_add_check_constraint(pParse,&E);}
 tcons ::= FOREIGN KEY LP eidlist(FA) RP
           REFERENCES nm(T) eidlist_opt(TA) refargs(R) defer_subclause_opt(D). {
     sqlite3CreateForeignKey(pParse, FA, &T, TA, R);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index a7ef80f..a1a5269 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3532,7 +3532,14 @@ void sqlite3StartTable(Parse *, Token *, int);
 void sqlite3AddColumn(Parse *, Token *, Token *);
 void sqlite3AddNotNull(Parse *, int);
 void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);
-void sqlite3AddCheckConstraint(Parse *, Expr *);
+
+/**
+ * Add a new CHECK constraint to the table currently under construction.
+ * @param parser Parsing context.
+ * @param span parser parsed expression object..
+ */
+void sql_add_check_constraint(Parse *parser, ExprSpan *span);
+
 void sqlite3AddDefaultValue(Parse *, ExprSpan *);
 void sqlite3AddCollateType(Parse *, Token *);
 
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> As we would store Check source expr in MsgPack we need
> span instead of parsed Expr only.
> Renamed refactored function to sql_add_check_constraint.
> 
> Part of #3272.

OK to push.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Vladislav Shpilevoy]

Thanks for the patch! See 6 comments below.

On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> As we would store Check source expr in MsgPack we need
> span instead of parsed Expr only.
> Renamed refactored function to sql_add_check_constraint.

1. Renamed refactored?

> 
> Part of #3272.
> ---
>   src/box/sql/build.c     | 24 ++++++++++--------------
>   src/box/sql/parse.y     |  4 ++--
>   src/box/sql/sqliteInt.h |  9 ++++++++-
>   3 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a6ddcf0..afb1128 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1019,26 +1019,22 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
>   	return;
>   }
>   
> -/*
> - * Add a new CHECK constraint to the table currently under construction.
> - */
>   void
> -sqlite3AddCheckConstraint(Parse * pParse,	/* Parsing context */
> -			  Expr * pCheckExpr	/* The check expression */
> -    )
> +sql_add_check_constraint(Parse *parser, ExprSpan *span)
>   {
>   #ifndef SQLITE_OMIT_CHECK

2. This macro is not defined. You can remove it.

> -	Table *pTab = pParse->pNewTable;
> -	if (pTab) {
> -		pTab->pCheck =
> -		    sqlite3ExprListAppend(pParse, pTab->pCheck, pCheckExpr);
> -		if (pParse->constraintName.n) {
> -			sqlite3ExprListSetName(pParse, pTab->pCheck,
> -					       &pParse->constraintName, 1);
> +	struct Expr *expr = span->pExpr;
> +	Table *table = parser->pNewTable;
> +	if (table != NULL) {
> +		table->pCheck =
> +			sqlite3ExprListAppend(parser, table->pCheck, expr);
> +		if (parser->constraintName.n) {

3. != 0

> +			sqlite3ExprListSetName(parser, table->pCheck,
> +					       &parser->constraintName, 1);
>   		}
>   	} else
>   #endif
> -		sql_expr_delete(pParse->db, pCheckExpr, false);
> +		sql_expr_delete(parser->db, expr, false);

4. If 'if' body is in {}, then else must be too.

>   }
>   
>   /*
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index a7ef80f..a1a5269 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -3532,7 +3532,14 @@ void sqlite3StartTable(Parse *, Token *, int);
>   void sqlite3AddColumn(Parse *, Token *, Token *);
>   void sqlite3AddNotNull(Parse *, int);
>   void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);
> -void sqlite3AddCheckConstraint(Parse *, Expr *);
> +
> +/**
> + * Add a new CHECK constraint to the table currently under construction.
> + * @param parser Parsing context.
> + * @param span parser parsed expression object..

5. span parser? And the span is not parsed. You said it in the commit message:
"we need span instead of parsed Expr".

> + */
> +void sql_add_check_constraint(Parse *parser, ExprSpan *span);

6. Wrap line after return type declaration.

> +
>   void sqlite3AddDefaultValue(Parse *, ExprSpan *);
>   void sqlite3AddCollateType(Parse *, Token *);
>   
> 

[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Kirill Shcherbatov]

> 1. Renamed refactored?
sql: change sqlite3AddCheckConstraint signature                                                                                             

As we would store Check source expr in MsgPack we need                                                                                                                      
span instead of parsed Expr in sqlite3AddCheckConstraint.                                                                                                                  
                                                                                                                                                                            
Part of #3272.↵

> 2. This macro is not defined. You can remove it.
> 3. != 0
> 4. If 'if' body is in {}, then else must be too.

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index afb1128..50e4985 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1022,19 +1022,19 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 void
 sql_add_check_constraint(Parse *parser, ExprSpan *span)
 {
-#ifndef SQLITE_OMIT_CHECK
 	struct Expr *expr = span->pExpr;
 	Table *table = parser->pNewTable;
 	if (table != NULL) {
 		table->pCheck =
 			sqlite3ExprListAppend(parser, table->pCheck, expr);
-		if (parser->constraintName.n) {
+		if (parser->constraintName.n != 0) {
 			sqlite3ExprListSetName(parser, table->pCheck,
 					       &parser->constraintName, 1);
 		}
-	} else
-#endif
+	} else {
 		sql_expr_delete(parser->db, expr, false);
+
+	}
 }
 
 /*

> 5. span parser? And the span is not parsed. You said it in the commit message:
> "we need span instead of parsed Expr".

- * @param span parser parsed expression object..
+ * @param check expression span object.

> 6. Wrap line after return type declaration.
-void sql_add_check_constraint(Parse *parser, ExprSpan *span);
+void
+sql_add_check_constraint(Parse *parser, ExprSpan *span);

[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[n.pettik]

Hello. I have checked out last version on branch.
I can provide only a few codestyle nitpickings.

>--- a/src/box/sql/build.c
>+++ b/src/box/sql/build.c
>@@ -1019,26 +1019,22 @@ sqlite3AddPrimaryKey(Parse * pParse,    /* Parsing context */
>        return;
>}
> 
>-/*
>- * Add a new CHECK constraint to the table currently under construction.
>- */
>void
>-sqlite3AddCheckConstraint(Parse * pParse,      /* Parsing context */
>-                         Expr * pCheckExpr     /* The check expression */
>-    )
>+sql_add_check_constraint(Parse *parser, ExprSpan *span)

Lets use ‘struct’ prefix, as it should be according to codestyle.

>+       Table *table = parser->pNewTable;

The same.

>+++ b/src/box/sql/sqliteInt.h
>@@ -3504,7 +3504,15 @@ void sqlite3StartTable(Parse *, Token *, int);
> void sqlite3AddColumn(Parse *, Token *, Token *);
> void sqlite3AddNotNull(Parse *, int);
> void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);
>-void sqlite3AddCheckConstraint(Parse *, Expr *);
>+
>+/**
>+ * Add a new CHECK constraint to the table currently under construction.

Out of 66 chars.

[tarantool-patches] Re: [PATCH v7 5/7] sql: change sqlite3AddCheckConstraint signature

[Kirill Shcherbatov]

> Lets use ‘struct’ prefix, as it should be according to codestyle.
> The same.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 50e4985..db8e0ce 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1020,10 +1020,10 @@ sqlite3AddPrimaryKey(Parse * pParse,    /* Parsing context */
 }
 
 void
-sql_add_check_constraint(Parse *parser, ExprSpan *span)
+sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span)
 {
        struct Expr *expr = span->pExpr;
-       Table *table = parser->pNewTable;
+       struct Table *table = parser->pNewTable;
        if (table != NULL) {
                table->pCheck =
                        sqlite3ExprListAppend(parser, table->pCheck, expr);

>> + * Add a new CHECK constraint to the table currently under construction.
> 
> Out of 66 chars.
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 10732aa..87ebbfb 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3506,7 +3506,8 @@ void sqlite3AddNotNull(Parse *, int);
void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order);

/**
- * Add a new CHECK constraint to the table currently under construction.
+ * Add a new CHECK constraint to the table currently under
+ * construction.
* @param parser Parsing context.
* @param span Expression span object.
*/

[tarantool-patches] [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Kirill Shcherbatov]

As we would like work with check in server, we need to export
- sqlite3ExprListAppend as sql_expr_list_append
- sqlite3ExprListDelete as sql_expr_list_delete
- sqlite3ExprListDup as sql_expr_list_dup.

Part of #3272.
---
 src/box/sql.h           | 57 ++++++++++++++++++++++++++++++
 src/box/sql/build.c     | 39 +++++++++++----------
 src/box/sql/expr.c      | 93 +++++++++++++++++++++----------------------------
 src/box/sql/fkey.c      | 11 +++---
 src/box/sql/insert.c    |  2 +-
 src/box/sql/parse.y     | 81 +++++++++++++++++++++---------------------
 src/box/sql/prepare.c   |  2 +-
 src/box/sql/resolve.c   | 37 +++++++-------------
 src/box/sql/select.c    | 40 +++++++++++----------
 src/box/sql/sqliteInt.h |  4 ---
 src/box/sql/trigger.c   |  8 ++---
 src/box/sql/update.c    |  2 +-
 src/box/sql/wherecode.c | 12 +++----
 src/box/sql/whereexpr.c |  6 ++--
 14 files changed, 214 insertions(+), 180 deletions(-)

diff --git a/src/box/sql.h b/src/box/sql.h
index 20f0651..d031d9b 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -178,6 +178,63 @@ int
 sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
 
 /**
+ * Duplicate Expr list.
+ * The flags parameter contains a combination of the EXPRDUP_XXX flags.
+ * If the EXPRDUP_REDUCE flag is set, then the structure returned is a
+ * truncated version of the usual Expr structure that will be stored as
+ * part of the in-memory representation of the database schema.
+ * @param db The database connection.
+ * @param p The ExprList to duplicate.
+ * @param flags EXPRDUP_XXX flags.
+ * @retval NULL on memory allocation error.
+ * @retval not NULL on success.
+ */
+struct ExprList *
+sql_expr_list_dup(struct sqlite3 * db, struct ExprList * p, int flags);
+
+/**
+ * Free AST pointed by expr list.
+ * @param db SQL handle.
+ * @param expr_list Root pointer of ExprList.
+ */
+void
+sql_expr_list_delete(struct sqlite3 *db, struct ExprList *expr_list);
+
+/**
+ * Add a new element to the end of an expression list.  If expr_list is
+ * 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:
+ * *   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 parser Parsing context.
+ * @param table The table being referenced.
+ * @param type NC_IsCheck or NC_PartIdx or NC_IdxExpr.
+ * @param expr Expression to resolve.  May be NULL.
+ * @param expr_list Expression list to resolve.  May be NUL.
+ */
+void
+sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
+			   struct Expr *expr, struct ExprList *expr_list);
+
+/**
  * Initialize a new parser object.
  * A number of service allocations are performed on the region, which is also
  * cleared in the destroy function.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index afb1128..d00bb1d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -252,7 +252,7 @@ static void
 freeIndex(sqlite3 * db, Index * p)
 {
 	sql_expr_delete(db, p->pPartIdxWhere, false);
-	sqlite3ExprListDelete(db, p->aColExpr);
+	sql_expr_list_delete(db, p->aColExpr);
 	sqlite3DbFree(db, p->zColAff);
 	sqlite3DbFree(db, p);
 }
@@ -406,7 +406,7 @@ deleteTable(sqlite3 * db, Table * pTable)
 	sqlite3DbFree(db, pTable->aCol);
 	sqlite3DbFree(db, pTable->zColAff);
 	sqlite3SelectDelete(db, pTable->pSelect);
-	sqlite3ExprListDelete(db, pTable->pCheck);
+	sql_expr_list_delete(db, pTable->pCheck);
 	assert(pTable->def != NULL);
 	/* Do not delete pTable->def allocated on region. */
 	if (!pTable->def->opts.temporary)
@@ -1015,7 +1015,7 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 	}
 
  primary_key_exit:
-	sqlite3ExprListDelete(pParse->db, pList);
+	sql_expr_list_delete(pParse->db, pList);
 	return;
 }
 
@@ -1027,7 +1027,7 @@ sql_add_check_constraint(Parse *parser, ExprSpan *span)
 	Table *table = parser->pNewTable;
 	if (table != NULL) {
 		table->pCheck =
-			sqlite3ExprListAppend(parser, table->pCheck, expr);
+			sql_expr_list_append(parser->db, table->pCheck, expr);
 		if (parser->constraintName.n) {
 			sqlite3ExprListSetName(parser, table->pCheck,
 					       &parser->constraintName, 1);
@@ -1411,7 +1411,7 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 		ExprList *pList;
 		Token ipkToken;
 		sqlite3TokenInit(&ipkToken, pTab->def->fields[pTab->iPKey].name);
-		pList = sqlite3ExprListAppend(pParse, 0,
+		pList = sql_expr_list_append(pParse->db, NULL,
 					      sqlite3ExprAlloc(db, TK_ID,
 							       &ipkToken, 0));
 		if (pList == 0)
@@ -1855,7 +1855,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 	/* Resolve names in all CHECK constraint expressions.
 	 */
 	if (p->pCheck) {
-		sqlite3ResolveSelfReference(pParse, p, NC_IsCheck, 0,
+		sql_resolve_self_reference(pParse, p, NC_IsCheck, NULL,
 					    p->pCheck);
 	}
 #endif				/* !defined(SQLITE_OMIT_CHECK) */
@@ -2020,7 +2020,7 @@ sqlite3CreateView(Parse * pParse,	/* The parsing context */
 	 */
 	p->pSelect = sqlite3SelectDup(db, pSelect, EXPRDUP_REDUCE);
 	p->def->opts.is_view = true;
-	p->pCheck = sqlite3ExprListDup(db, pCNames, EXPRDUP_REDUCE);
+	p->pCheck = sql_expr_list_dup(db, pCNames, EXPRDUP_REDUCE);
 	if (db->mallocFailed)
 		goto create_view_fail;
 
@@ -2047,7 +2047,7 @@ sqlite3CreateView(Parse * pParse,	/* The parsing context */
 
  create_view_fail:
 	sqlite3SelectDelete(db, pSelect);
-	sqlite3ExprListDelete(db, pCNames);
+	sql_expr_list_delete(db, pCNames);
 	return;
 }
 #endif				/* SQLITE_OMIT_VIEW */
@@ -2561,8 +2561,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_delete(db, pFromCol);
+	sql_expr_list_delete(db, pToCol);
 }
 
 /*
@@ -2952,7 +2952,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 		Token prevCol;
 		sqlite3TokenInit(&prevCol, pTab->def->fields[
 			pTab->def->field_count - 1].name);
-		col_list = sqlite3ExprListAppend(parse, 0,
+		col_list = sql_expr_list_append(parse->db, NULL,
 						 sqlite3ExprAlloc(db, TK_ID,
 								  &prevCol, 0));
 		if (col_list == NULL)
@@ -3005,7 +3005,8 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	pIndex->nColumn = col_list->nExpr;
 	/* Tarantool have access to each column by any index */
 	if (where) {
-		sqlite3ResolveSelfReference(parse, pTab, NC_PartIdx, where, 0);
+		sql_resolve_self_reference(parse, pTab, NC_PartIdx, where,
+					   NULL);
 		pIndex->pPartIdxWhere = where;
 		where = NULL;
 	}
@@ -3022,8 +3023,8 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	for (i = 0, col_listItem = col_list->a; i < col_list->nExpr;
 	     i++, col_listItem++) {
 		Expr *pCExpr;	/* The i-th index expression */
-		sqlite3ResolveSelfReference(parse, pTab, NC_IdxExpr,
-					    col_listItem->pExpr, 0);
+		sql_resolve_self_reference(parse, pTab, NC_IdxExpr,
+					    col_listItem->pExpr, NULL);
 		if (parse->nErr > 0)
 			goto exit_create_index;
 		pCExpr = sqlite3ExprSkipCollate(col_listItem->pExpr);
@@ -3236,7 +3237,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	if (pIndex)
 		freeIndex(db, pIndex);
 	sql_expr_delete(db, where, false);
-	sqlite3ExprListDelete(db, col_list);
+	sql_expr_list_delete(db, col_list);
 	sqlite3SrcListDelete(db, tbl_name);
 	sqlite3DbFree(db, zName);
 }
@@ -3648,7 +3649,7 @@ sqlite3SrcListDelete(sqlite3 * db, SrcList * pList)
 		if (pItem->fg.isIndexedBy)
 			sqlite3DbFree(db, pItem->u1.zIndexedBy);
 		if (pItem->fg.isTabFunc)
-			sqlite3ExprListDelete(db, pItem->u1.pFuncArg);
+			sql_expr_list_delete(db, pItem->u1.pFuncArg);
 		sqlite3DeleteTable(db, pItem->pTab);
 		sqlite3SelectDelete(db, pItem->pSelect);
 		sql_expr_delete(db, pItem->pOn, false);
@@ -3754,7 +3755,7 @@ sqlite3SrcListFuncArgs(Parse * pParse, SrcList * p, ExprList * pList)
 		pItem->u1.pFuncArg = pList;
 		pItem->fg.isTabFunc = 1;
 	} else {
-		sqlite3ExprListDelete(pParse->db, pList);
+		sql_expr_list_delete(pParse->db, pList);
 	}
 }
 
@@ -4110,7 +4111,7 @@ sqlite3WithAdd(Parse * pParse,	/* Parsing context */
 	assert((pNew != 0 && zName != 0) || db->mallocFailed);
 
 	if (db->mallocFailed) {
-		sqlite3ExprListDelete(db, pArglist);
+		sql_expr_list_delete(db, pArglist);
 		sqlite3SelectDelete(db, pQuery);
 		sqlite3DbFree(db, zName);
 		pNew = pWith;
@@ -4135,7 +4136,7 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
 		int i;
 		for (i = 0; i < pWith->nCte; i++) {
 			struct Cte *pCte = &pWith->a[i];
-			sqlite3ExprListDelete(db, pCte->pCols);
+			sql_expr_list_delete(db, pCte->pCols);
 			sqlite3SelectDelete(db, pCte->pSelect);
 			sqlite3DbFree(db, pCte->zName);
 		}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 64991ee..ecb0915 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1043,7 +1043,7 @@ sqlite3ExprFunction(Parse * pParse, ExprList * pList, Token * pToken)
 	assert(pToken);
 	pNew = sqlite3ExprAlloc(db, TK_FUNCTION, pToken, 1);
 	if (pNew == 0) {
-		sqlite3ExprListDelete(db, pList);	/* Avoid memory leak when malloc fails */
+		sql_expr_list_delete(db, pList);	/* Avoid memory leak when malloc fails */
 		return 0;
 	}
 	pNew->x.pList = pList;
@@ -1162,7 +1162,7 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p, bool extern_alloc)
 		if (ExprHasProperty(p, EP_xIsSelect)) {
 			sqlite3SelectDelete(db, p->x.pSelect);
 		} else {
-			sqlite3ExprListDelete(db, p->x.pList);
+			sql_expr_list_delete(db, p->x.pList);
 		}
 	}
 	if (ExprHasProperty(p, EP_MemToken))
@@ -1350,8 +1350,7 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer)
 						     flags);
 			} else {
 				pNew->x.pList =
-				    sqlite3ExprListDup(db, p->x.pList,
-						       flags);
+					sql_expr_list_dup(db, p->x.pList, flags);
 			}
 		}
 
@@ -1408,7 +1407,7 @@ withDup(sqlite3 * db, With * p)
 				pRet->a[i].pSelect =
 				    sqlite3SelectDup(db, p->a[i].pSelect, 0);
 				pRet->a[i].pCols =
-				    sqlite3ExprListDup(db, p->a[i].pCols, 0);
+				    sql_expr_list_dup(db, p->a[i].pCols, 0);
 				pRet->a[i].zName =
 				    sqlite3DbStrDup(db, p->a[i].zName);
 			}
@@ -1426,7 +1425,7 @@ withDup(sqlite3 * db, With * p)
  * be deleted (by being passed to their respective ...Delete() routines)
  * without effecting the originals.
  *
- * The expression list, ID, and source lists return by sqlite3ExprListDup(),
+ * The expression list, ID, and source lists return by sql_expr_list_dup(),
  * sqlite3IdListDup(), and sqlite3SrcListDup() can not be further expanded
  * by subsequent calls to sqlite*ListAppend() routines.
  *
@@ -1444,8 +1443,8 @@ sqlite3ExprDup(sqlite3 * db, Expr * p, int flags)
 	return p ? sql_expr_dup(db, p, flags, 0) : 0;
 }
 
-ExprList *
-sqlite3ExprListDup(sqlite3 * db, ExprList * p, int flags)
+struct ExprList *
+sql_expr_list_dup(struct sqlite3 * db, struct ExprList *p, int flags)
 {
 	ExprList *pNew;
 	struct ExprList_item *pItem, *pOldItem;
@@ -1538,7 +1537,7 @@ sqlite3SrcListDup(sqlite3 * db, SrcList * p, int flags)
 		pNewItem->pIBIndex = pOldItem->pIBIndex;
 		if (pNewItem->fg.isTabFunc) {
 			pNewItem->u1.pFuncArg =
-			    sqlite3ExprListDup(db, pOldItem->u1.pFuncArg,
+			    sql_expr_list_dup(db, pOldItem->u1.pFuncArg,
 					       flags);
 		}
 		pTab = pNewItem->pTab = pOldItem->pTab;
@@ -1594,12 +1593,12 @@ sqlite3SelectDup(sqlite3 * db, Select * p, int flags)
 	pNew = sqlite3DbMallocRawNN(db, sizeof(*p));
 	if (pNew == 0)
 		return 0;
-	pNew->pEList = sqlite3ExprListDup(db, p->pEList, flags);
+	pNew->pEList = sql_expr_list_dup(db, p->pEList, flags);
 	pNew->pSrc = sqlite3SrcListDup(db, p->pSrc, flags);
 	pNew->pWhere = sqlite3ExprDup(db, p->pWhere, flags);
-	pNew->pGroupBy = sqlite3ExprListDup(db, p->pGroupBy, flags);
+	pNew->pGroupBy = sql_expr_list_dup(db, p->pGroupBy, flags);
 	pNew->pHaving = sqlite3ExprDup(db, p->pHaving, flags);
-	pNew->pOrderBy = sqlite3ExprListDup(db, p->pOrderBy, flags);
+	pNew->pOrderBy = sql_expr_list_dup(db, p->pOrderBy, flags);
 	pNew->op = p->op;
 	pNew->pPrior = pPrior = sqlite3SelectDup(db, p->pPrior, flags);
 	if (pPrior)
@@ -1618,51 +1617,39 @@ sqlite3SelectDup(sqlite3 * db, Select * p, int flags)
 	return pNew;
 }
 
-/*
- * Add a new element to the end of an expression list.  If pList is
- * 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.
- */
-ExprList *
-sqlite3ExprListAppend(Parse * pParse,	/* Parsing context */
-		      ExprList * pList,	/* List to which to append. Might be NULL */
-		      Expr * pExpr	/* Expression to be appended. Might be NULL */
-    )
+struct ExprList *
+sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
+	struct Expr *expr)
 {
-	sqlite3 *db = pParse->db;
-	assert(db != 0);
-	if (pList == 0) {
-		pList = sqlite3DbMallocRawNN(db, sizeof(ExprList));
-		if (pList == 0) {
+	assert(db != NULL);
+	if (expr_list == NULL) {
+		expr_list = sqlite3DbMallocRawNN(db, sizeof(ExprList));
+		if (expr_list == NULL)
 			goto no_mem;
-		}
-		pList->nExpr = 0;
-		pList->a = sqlite3DbMallocRawNN(db, sizeof(pList->a[0]));
-		if (pList->a == 0)
+		expr_list->nExpr = 0;
+		expr_list->a =
+			sqlite3DbMallocRawNN(db, sizeof(expr_list->a[0]));
+		if (expr_list->a == NULL)
 			goto no_mem;
-	} else if ((pList->nExpr & (pList->nExpr - 1)) == 0) {
+	} else if ((expr_list->nExpr & (expr_list->nExpr - 1)) == 0) {
 		struct ExprList_item *a;
-		assert(pList->nExpr > 0);
-		a = sqlite3DbRealloc(db, pList->a,
-				     pList->nExpr * 2 * sizeof(pList->a[0]));
-		if (a == 0) {
+		assert(expr_list->nExpr > 0);
+		a = sqlite3DbRealloc(db, expr_list->a, expr_list->nExpr * 2 *
+				     sizeof(expr_list->a[0]));
+		if (a == NULL)
 			goto no_mem;
-		}
-		pList->a = a;
+		expr_list->a = a;
 	}
-	assert(pList->a != 0);
-	struct ExprList_item *pItem = &pList->a[pList->nExpr++];
+	assert(expr_list->a != NULL);
+	struct ExprList_item *pItem = &expr_list->a[expr_list->nExpr++];
 	memset(pItem, 0, sizeof(*pItem));
-	pItem->pExpr = pExpr;
-	return pList;
+	pItem->pExpr = expr;
+	return expr_list;
 
  no_mem:
 	/* Avoid leaking memory if malloc has failed. */
-	sql_expr_delete(db, pExpr, false);
-	sqlite3ExprListDelete(db, pList);
+	sql_expr_delete(db, expr, false);
+	sql_expr_list_delete(db, expr_list);
 	return 0;
 }
 
@@ -1710,7 +1697,7 @@ sqlite3ExprListAppendVector(Parse * pParse,	/* Parsing context */
 
 	for (i = 0; i < pColumns->nId; i++) {
 		Expr *pSubExpr = sqlite3ExprForVectorField(pParse, pExpr, i);
-		pList = sqlite3ExprListAppend(pParse, pList, pSubExpr);
+		pList = sql_expr_list_append(pParse->db, pList, pSubExpr);
 		if (pList) {
 			assert(pList->nExpr == iFirst + i + 1);
 			pList->a[pList->nExpr - 1].zName = pColumns->a[i].zName;
@@ -1724,7 +1711,7 @@ sqlite3ExprListAppendVector(Parse * pParse,	/* Parsing context */
 			assert(pFirst->op == TK_SELECT_COLUMN);
 
 			/* Store the SELECT statement in pRight so it will be deleted when
-			 * sqlite3ExprListDelete() is called
+			 * sql_expr_list_delete() is called
 			 */
 			pFirst->pRight = pExpr;
 			pExpr = 0;
@@ -1847,10 +1834,10 @@ exprListDeleteNN(sqlite3 * db, ExprList * pList)
 }
 
 void
-sqlite3ExprListDelete(sqlite3 * db, ExprList * pList)
+sql_expr_list_delete(sqlite3 *db, ExprList *expr_list)
 {
-	if (pList)
-		exprListDeleteNN(db, pList);
+	if (expr_list)
+		exprListDeleteNN(db, expr_list);
 }
 
 /*
@@ -4394,7 +4381,7 @@ sqlite3ExprCodeAtInit(Parse * pParse,	/* Parsing context */
 	assert(ConstFactorOk(pParse));
 	p = pParse->pConstExpr;
 	pExpr = sqlite3ExprDup(pParse->db, pExpr, 0);
-	p = sqlite3ExprListAppend(pParse, p, pExpr);
+	p = sql_expr_list_append(pParse->db, p, pExpr);
 	if (p) {
 		struct ExprList_item *pItem = &p->a[p->nExpr - 1];
 		pItem->u.iConstExprReg = regDest;
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 2ab8fdd..3e30a44 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -743,7 +743,7 @@ fkTriggerDelete(sqlite3 * dbMem, Trigger * p)
 	if (p) {
 		TriggerStep *pStep = p->step_list;
 		sql_expr_delete(dbMem, pStep->pWhere, false);
-		sqlite3ExprListDelete(dbMem, pStep->pExprList);
+		sql_expr_list_delete(dbMem, pStep->pExprList);
 		sqlite3SelectDelete(dbMem, pStep->pSelect);
 		sql_expr_delete(dbMem, p->pWhen, false);
 		sqlite3DbFree(dbMem, p);
@@ -1353,7 +1353,8 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 					    sqlite3ExprAlloc(db, TK_NULL, 0, 0);
 				}
 				pList =
-				    sqlite3ExprListAppend(pParse, pList, pNew);
+				    sql_expr_list_append(pParse->db, pList,
+							 pNew);
 				sqlite3ExprListSetName(pParse, pList, &tFromCol,
 						       0);
 			}
@@ -1376,7 +1377,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 				pRaise->affinity = ON_CONFLICT_ACTION_ABORT;
 			}
 			pSelect = sqlite3SelectNew(pParse,
-						   sqlite3ExprListAppend(pParse,
+						   sql_expr_list_append(pParse->db,
 									 0,
 									 pRaise),
 						   sqlite3SrcListAppend(db, 0,
@@ -1401,7 +1402,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 			pStep->pWhere =
 			    sqlite3ExprDup(db, pWhere, EXPRDUP_REDUCE);
 			pStep->pExprList =
-			    sqlite3ExprListDup(db, pList, EXPRDUP_REDUCE);
+			    sql_expr_list_dup(db, pList, EXPRDUP_REDUCE);
 			pStep->pSelect =
 			    sqlite3SelectDup(db, pSelect, EXPRDUP_REDUCE);
 			if (pWhen) {
@@ -1416,7 +1417,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 
 		sql_expr_delete(db, pWhere, false);
 		sql_expr_delete(db, pWhen, false);
-		sqlite3ExprListDelete(db, pList);
+		sql_expr_list_delete(db, pList);
 		sqlite3SelectDelete(db, pSelect);
 		if (db->mallocFailed == 1) {
 			fkTriggerDelete(db, pTrigger);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6cc6805..6e7144c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -919,7 +919,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 
  insert_cleanup:
 	sqlite3SrcListDelete(db, pTabList);
-	sqlite3ExprListDelete(db, pList);
+	sql_expr_list_delete(db, pList);
 	sqlite3SelectDelete(db, pSelect);
 	sqlite3IdListDelete(db, pColumn);
 	sqlite3DbFree(db, aRegIdx);
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index cc3931d..6f385ba 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -528,25 +528,25 @@ distinct(A) ::= .           {A = 0;}
 // opcode of TK_ASTERISK.
 //
 %type selcollist {ExprList*}
-%destructor selcollist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor selcollist {sql_expr_list_delete(pParse->db, $$);}
 %type sclp {ExprList*}
-%destructor sclp {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor sclp {sql_expr_list_delete(pParse->db, $$);}
 sclp(A) ::= selcollist(A) COMMA.
 sclp(A) ::= .                                {A = 0;}
 selcollist(A) ::= sclp(A) expr(X) as(Y).     {
-   A = sqlite3ExprListAppend(pParse, A, X.pExpr);
+   A = sql_expr_list_append(pParse->db, A, X.pExpr);
    if( Y.n>0 ) sqlite3ExprListSetName(pParse, A, &Y, 1);
    sqlite3ExprListSetSpan(pParse,A,&X);
 }
 selcollist(A) ::= sclp(A) STAR. {
   Expr *p = sqlite3Expr(pParse->db, TK_ASTERISK, 0);
-  A = sqlite3ExprListAppend(pParse, A, p);
+  A = sql_expr_list_append(pParse->db, A, p);
 }
 selcollist(A) ::= sclp(A) nm(X) DOT STAR. {
   Expr *pRight = sqlite3PExpr(pParse, TK_ASTERISK, 0, 0);
   Expr *pLeft = sqlite3ExprAlloc(pParse->db, TK_ID, &X, 1);
   Expr *pDot = sqlite3PExpr(pParse, TK_DOT, pLeft, pRight);
-  A = sqlite3ExprListAppend(pParse,A, pDot);
+  A = sql_expr_list_append(pParse->db,A, pDot);
 }
 
 // An option "AS <id>" phrase that can follow one of the expressions that
@@ -661,23 +661,24 @@ using_opt(U) ::= .                        {U = 0;}
 
 
 %type orderby_opt {ExprList*}
-%destructor orderby_opt {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor orderby_opt {sql_expr_list_delete(pParse->db, $$);}
 
 // the sortlist non-terminal stores a list of expression where each
 // expression is optionally followed by ASC or DESC to indicate the
 // sort order.
 //
 %type sortlist {ExprList*}
-%destructor sortlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor sortlist {sql_expr_list_delete(pParse->db, $$);}
 
 orderby_opt(A) ::= .                          {A = 0;}
 orderby_opt(A) ::= ORDER BY sortlist(X).      {A = X;}
 sortlist(A) ::= sortlist(A) COMMA expr(Y) sortorder(Z). {
-  A = sqlite3ExprListAppend(pParse,A,Y.pExpr);
+  A = sql_expr_list_append(pParse->db,A,Y.pExpr);
   sqlite3ExprListSetSortOrder(A,Z);
 }
 sortlist(A) ::= expr(Y) sortorder(Z). {
-  A = sqlite3ExprListAppend(pParse,0,Y.pExpr); /*A-overwrites-Y*/
+  /* A-overwrites-Y. */
+  A = sql_expr_list_append(pParse->db,NULL,Y.pExpr);
   sqlite3ExprListSetSortOrder(A,Z);
 }
 
@@ -688,7 +689,7 @@ sortorder(A) ::= DESC.          {A = SORT_ORDER_DESC;}
 sortorder(A) ::= .              {A = SORT_ORDER_UNDEF;}
 
 %type groupby_opt {ExprList*}
-%destructor groupby_opt {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor groupby_opt {sql_expr_list_delete(pParse->db, $$);}
 groupby_opt(A) ::= .                      {A = 0;}
 groupby_opt(A) ::= GROUP BY nexprlist(X). {A = X;}
 
@@ -750,17 +751,17 @@ cmd ::= with(C) UPDATE orconf(R) fullname(X) indexed_opt(I) SET setlist(Y)
 %endif
 
 %type setlist {ExprList*}
-%destructor setlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor setlist {sql_expr_list_delete(pParse->db, $$);}
 
 setlist(A) ::= setlist(A) COMMA nm(X) EQ expr(Y). {
-  A = sqlite3ExprListAppend(pParse, A, Y.pExpr);
+  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
   sqlite3ExprListSetName(pParse, A, &X, 1);
 }
 setlist(A) ::= setlist(A) COMMA LP idlist(X) RP EQ expr(Y). {
   A = sqlite3ExprListAppendVector(pParse, A, X, Y.pExpr);
 }
 setlist(A) ::= nm(X) EQ expr(Y). {
-  A = sqlite3ExprListAppend(pParse, 0, Y.pExpr);
+  A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
   sqlite3ExprListSetName(pParse, A, &X, 1);
 }
 setlist(A) ::= LP idlist(X) RP EQ expr(Y). {
@@ -942,13 +943,13 @@ term(A) ::= CTIME_KW(OP). {
 }
 
 expr(A) ::= LP(L) nexprlist(X) COMMA expr(Y) RP(R). {
-  ExprList *pList = sqlite3ExprListAppend(pParse, X, Y.pExpr);
+  ExprList *pList = sql_expr_list_append(pParse->db, X, Y.pExpr);
   A.pExpr = sqlite3PExpr(pParse, TK_VECTOR, 0, 0);
   if( A.pExpr ){
     A.pExpr->x.pList = pList;
     spanSet(&A, &L, &R);
   }else{
-    sqlite3ExprListDelete(pParse->db, pList);
+    sql_expr_list_delete(pParse->db, pList);
   }
 }
 
@@ -971,8 +972,8 @@ expr(A) ::= expr(A) likeop(OP) expr(Y).  [LIKE_KW]  {
   ExprList *pList;
   int bNot = OP.n & 0x80000000;
   OP.n &= 0x7fffffff;
-  pList = sqlite3ExprListAppend(pParse,0, Y.pExpr);
-  pList = sqlite3ExprListAppend(pParse,pList, A.pExpr);
+  pList = sql_expr_list_append(pParse->db,NULL, Y.pExpr);
+  pList = sql_expr_list_append(pParse->db,pList, A.pExpr);
   A.pExpr = sqlite3ExprFunction(pParse, pList, &OP);
   exprNot(pParse, bNot, &A);
   A.zEnd = Y.zEnd;
@@ -982,9 +983,9 @@ expr(A) ::= expr(A) likeop(OP) expr(Y) ESCAPE expr(E).  [LIKE_KW]  {
   ExprList *pList;
   int bNot = OP.n & 0x80000000;
   OP.n &= 0x7fffffff;
-  pList = sqlite3ExprListAppend(pParse,0, Y.pExpr);
-  pList = sqlite3ExprListAppend(pParse,pList, A.pExpr);
-  pList = sqlite3ExprListAppend(pParse,pList, E.pExpr);
+  pList = sql_expr_list_append(pParse->db,NULL, Y.pExpr);
+  pList = sql_expr_list_append(pParse->db,pList, A.pExpr);
+  pList = sql_expr_list_append(pParse->db,pList, E.pExpr);
   A.pExpr = sqlite3ExprFunction(pParse, pList, &OP);
   exprNot(pParse, bNot, &A);
   A.zEnd = E.zEnd;
@@ -1067,13 +1068,13 @@ expr(A) ::= PLUS(B) expr(X). [BITNOT]
 between_op(A) ::= BETWEEN.     {A = 0;}
 between_op(A) ::= NOT BETWEEN. {A = 1;}
 expr(A) ::= expr(A) between_op(N) expr(X) AND expr(Y). [BETWEEN] {
-  ExprList *pList = sqlite3ExprListAppend(pParse,0, X.pExpr);
-  pList = sqlite3ExprListAppend(pParse,pList, Y.pExpr);
+  ExprList *pList = sql_expr_list_append(pParse->db,NULL, X.pExpr);
+  pList = sql_expr_list_append(pParse->db,pList, Y.pExpr);
   A.pExpr = sqlite3PExpr(pParse, TK_BETWEEN, A.pExpr, 0);
   if( A.pExpr ){
     A.pExpr->x.pList = pList;
   }else{
-    sqlite3ExprListDelete(pParse->db, pList);
+    sql_expr_list_delete(pParse->db, pList);
   } 
   exprNot(pParse, N, &A);
   A.zEnd = Y.zEnd;
@@ -1112,7 +1113,7 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
     */
     Expr *pRHS = Y->a[0].pExpr;
     Y->a[0].pExpr = 0;
-    sqlite3ExprListDelete(pParse->db, Y);
+    sql_expr_list_delete(pParse->db, Y);
     /* pRHS cannot be NULL because a malloc error would have been detected
     ** before now and control would have never reached this point */
     if( ALWAYS(pRHS) ){
@@ -1126,7 +1127,7 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
       A.pExpr->x.pList = Y;
       sqlite3ExprSetHeightAndFlags(pParse, A.pExpr);
     }else{
-      sqlite3ExprListDelete(pParse->db, Y);
+      sql_expr_list_delete(pParse->db, Y);
     }
     exprNot(pParse, N, &A);
   }
@@ -1164,22 +1165,22 @@ expr(A) ::= CASE(C) case_operand(X) case_exprlist(Y) case_else(Z) END(E). {
   spanSet(&A,&C,&E);  /*A-overwrites-C*/
   A.pExpr = sqlite3PExpr(pParse, TK_CASE, X, 0);
   if( A.pExpr ){
-    A.pExpr->x.pList = Z ? sqlite3ExprListAppend(pParse,Y,Z) : Y;
+    A.pExpr->x.pList = Z ? sql_expr_list_append(pParse->db,Y,Z) : Y;
     sqlite3ExprSetHeightAndFlags(pParse, A.pExpr);
   }else{
-    sqlite3ExprListDelete(pParse->db, Y);
+    sql_expr_list_delete(pParse->db, Y);
     sql_expr_delete(pParse->db, Z, false);
   }
 }
 %type case_exprlist {ExprList*}
-%destructor case_exprlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor case_exprlist {sql_expr_list_delete(pParse->db, $$);}
 case_exprlist(A) ::= case_exprlist(A) WHEN expr(Y) THEN expr(Z). {
-  A = sqlite3ExprListAppend(pParse,A, Y.pExpr);
-  A = sqlite3ExprListAppend(pParse,A, Z.pExpr);
+  A = sql_expr_list_append(pParse->db,A, Y.pExpr);
+  A = sql_expr_list_append(pParse->db,A, Z.pExpr);
 }
 case_exprlist(A) ::= WHEN expr(Y) THEN expr(Z). {
-  A = sqlite3ExprListAppend(pParse,0, Y.pExpr);
-  A = sqlite3ExprListAppend(pParse,A, Z.pExpr);
+  A = sql_expr_list_append(pParse->db,NULL, Y.pExpr);
+  A = sql_expr_list_append(pParse->db,A, Z.pExpr);
 }
 %type case_else {Expr*}
 %destructor case_else {sql_expr_delete(pParse->db, $$, false);}
@@ -1191,21 +1192,21 @@ case_operand(A) ::= expr(X).            {A = X.pExpr; /*A-overwrites-X*/}
 case_operand(A) ::= .                   {A = 0;} 
 
 %type exprlist {ExprList*}
-%destructor exprlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor exprlist {sql_expr_list_delete(pParse->db, $$);}
 %type nexprlist {ExprList*}
-%destructor nexprlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor nexprlist {sql_expr_list_delete(pParse->db, $$);}
 
 exprlist(A) ::= nexprlist(A).
 exprlist(A) ::= .                            {A = 0;}
 nexprlist(A) ::= nexprlist(A) COMMA expr(Y).
-    {A = sqlite3ExprListAppend(pParse,A,Y.pExpr);}
+    {A = sql_expr_list_append(pParse->db,A,Y.pExpr);}
 nexprlist(A) ::= expr(Y).
-    {A = sqlite3ExprListAppend(pParse,0,Y.pExpr); /*A-overwrites-Y*/}
+    {A = sql_expr_list_append(pParse->db,NULL,Y.pExpr); /*A-overwrites-Y*/}
 
 /* A paren_exprlist is an optional expression list contained inside
 ** of parenthesis */
 %type paren_exprlist {ExprList*}
-%destructor paren_exprlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor paren_exprlist {sql_expr_list_delete(pParse->db, $$);}
 paren_exprlist(A) ::= .   {A = 0;}
 paren_exprlist(A) ::= LP exprlist(X) RP.  {A = X;}
 
@@ -1232,9 +1233,9 @@ uniqueflag(A) ::= .        {A = ON_CONFLICT_ACTION_NONE;}
 // used for the arguments to an index.  That is just an historical accident.
 //
 %type eidlist {ExprList*}
-%destructor eidlist {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor eidlist {sql_expr_list_delete(pParse->db, $$);}
 %type eidlist_opt {ExprList*}
-%destructor eidlist_opt {sqlite3ExprListDelete(pParse->db, $$);}
+%destructor eidlist_opt {sql_expr_list_delete(pParse->db, $$);}
 
 %include {
   /* Add a single new term to an ExprList that is used to store a
@@ -1249,7 +1250,7 @@ uniqueflag(A) ::= .        {A = ON_CONFLICT_ACTION_NONE;}
     int hasCollate,
     int sortOrder
   ){
-    ExprList *p = sqlite3ExprListAppend(pParse, pPrior, 0);
+    ExprList *p = sql_expr_list_append(pParse->db, pPrior, NULL);
     if( (hasCollate || sortOrder != SORT_ORDER_UNDEF)
         && pParse->db->init.busy==0
     ){
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 8d94642..e7fc583 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -447,7 +447,7 @@ sql_parser_destroy(Parse *parser)
 	assert(parser != NULL);
 	sqlite3 *db = parser->db;
 	sqlite3DbFree(db, parser->aLabel);
-	sqlite3ExprListDelete(db, parser->pConstExpr);
+	sql_expr_list_delete(db, parser->pConstExpr);
 	if (db != NULL) {
 		assert(db->lookaside.bDisable >=
 		       parser->disableLookaside);
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 7f0858a..fd82119 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -1578,40 +1578,27 @@ sqlite3ResolveSelectNames(Parse * pParse,	/* The parser context */
 	sqlite3WalkSelect(&w, p);
 }
 
-/*
- * Resolve names in expressions that can only reference a single table:
- *
- *    *   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.
- */
 void
-sqlite3ResolveSelfReference(Parse * pParse,	/* Parsing context */
-			    Table * pTab,	/* The table being referenced */
-			    int type,	/* NC_IsCheck or NC_PartIdx or NC_IdxExpr */
-			    Expr * pExpr,	/* Expression to resolve.  May be NULL. */
-			    ExprList * pList	/* Expression list to resolve.  May be NUL. */
-    )
+sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
+			   struct Expr *expr, struct ExprList *expr_list)
 {
-	SrcList sSrc;		/* Fake SrcList for pParse->pNewTable */
-	NameContext sNC;	/* Name context for pParse->pNewTable */
+	/* Fake SrcList for pParse->pNewTable */
+	SrcList sSrc;
+	/* Name context for pParse->pNewTable */
+	NameContext sNC;
 
 	assert(type == NC_IsCheck || type == NC_PartIdx || type == NC_IdxExpr);
 	memset(&sNC, 0, sizeof(sNC));
 	memset(&sSrc, 0, sizeof(sSrc));
 	sSrc.nSrc = 1;
-	sSrc.a[0].zName = pTab->def->name;
-	sSrc.a[0].pTab = pTab;
+	sSrc.a[0].zName = table->def->name;
+	sSrc.a[0].pTab = table;
 	sSrc.a[0].iCursor = -1;
-	sNC.pParse = pParse;
+	sNC.pParse = parser;
 	sNC.pSrcList = &sSrc;
 	sNC.ncFlags = type;
-	if (sqlite3ResolveExprNames(&sNC, pExpr))
+	if (sqlite3ResolveExprNames(&sNC, expr))
 		return;
-	if (pList)
-		sqlite3ResolveExprListNames(&sNC, pList);
+	if (expr_list != NULL)
+		sqlite3ResolveExprListNames(&sNC, expr_list);
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 75bd53d..cd305be 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -95,12 +95,12 @@ clearSelect(sqlite3 * db, Select * p, int bFree)
 {
 	while (p) {
 		Select *pPrior = p->pPrior;
-		sqlite3ExprListDelete(db, p->pEList);
+		sql_expr_list_delete(db, p->pEList);
 		sqlite3SrcListDelete(db, p->pSrc);
 		sql_expr_delete(db, p->pWhere, false);
-		sqlite3ExprListDelete(db, p->pGroupBy);
+		sql_expr_list_delete(db, p->pGroupBy);
 		sql_expr_delete(db, p->pHaving, false);
-		sqlite3ExprListDelete(db, p->pOrderBy);
+		sql_expr_list_delete(db, p->pOrderBy);
 		sql_expr_delete(db, p->pLimit, false);
 		sql_expr_delete(db, p->pOffset, false);
 		if (p->pWith)
@@ -151,7 +151,7 @@ sqlite3SelectNew(Parse * pParse,	/* Parsing context */
 	}
 	if (pEList == 0) {
 		pEList =
-		    sqlite3ExprListAppend(pParse, 0,
+		    sql_expr_list_append(pParse->db, NULL,
 					  sqlite3Expr(db, TK_ASTERISK, 0));
 	}
 	struct session MAYBE_UNUSED *user_session;
@@ -2336,7 +2336,7 @@ generateWithRecursiveQuery(Parse * pParse,	/* Parsing context */
 	sqlite3VdbeResolveLabel(v, addrBreak);
 
  end_of_recursive_query:
-	sqlite3ExprListDelete(pParse->db, p->pOrderBy);
+	sql_expr_list_delete(pParse->db, p->pOrderBy);
 	p->pOrderBy = pOrderBy;
 	p->pLimit = pLimit;
 	p->pOffset = pOffset;
@@ -2629,7 +2629,7 @@ multiSelect(Parse * pParse,	/* Parsing context */
 				/* Query flattening in sqlite3Select() might refill p->pOrderBy.
 				 * Be sure to delete p->pOrderBy, therefore, to avoid a memory leak.
 				 */
-				sqlite3ExprListDelete(db, p->pOrderBy);
+				sql_expr_list_delete(db, p->pOrderBy);
 				pDelete = p->pPrior;
 				p->pPrior = pPrior;
 				p->pOrderBy = 0;
@@ -3191,7 +3191,7 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
 				pNew->flags |= EP_IntValue;
 				pNew->u.iValue = i;
 				pOrderBy =
-				    sqlite3ExprListAppend(pParse, pOrderBy,
+				    sql_expr_list_append(pParse->db, pOrderBy,
 							  pNew);
 				if (pOrderBy)
 					pOrderBy->a[nOrderBy++].u.x.
@@ -3224,7 +3224,7 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
 	/* Reattach the ORDER BY clause to the query.
 	 */
 	p->pOrderBy = pOrderBy;
-	pPrior->pOrderBy = sqlite3ExprListDup(pParse->db, pOrderBy, 0);
+	pPrior->pOrderBy = sql_expr_list_dup(pParse->db, pOrderBy, 0);
 
 	/* Allocate a range of temporary registers and the key_def needed
 	 * for the logic that removes duplicate result rows when the
@@ -4086,7 +4086,7 @@ flattenSubquery(Parse * pParse,		/* Parsing context */
 							  pParent->pHaving);
 			assert(pParent->pGroupBy == 0);
 			pParent->pGroupBy =
-			    sqlite3ExprListDup(db, pSub->pGroupBy, 0);
+			    sql_expr_list_dup(db, pSub->pGroupBy, 0);
 		} else {
 			pParent->pWhere =
 			    sqlite3ExprAnd(db, pWhere, pParent->pWhere);
@@ -4376,7 +4376,8 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p)
 	*pNew = *p;
 	p->pSrc = pNewSrc;
 	p->pEList =
-	    sqlite3ExprListAppend(pParse, 0, sqlite3Expr(db, TK_ASTERISK, 0));
+	    sql_expr_list_append(pParse->db, NULL,
+				 sqlite3Expr(db, TK_ASTERISK, 0));
 	p->op = TK_SELECT;
 	p->pWhere = 0;
 	pNew->pGroupBy = 0;
@@ -4817,7 +4818,7 @@ selectExpander(Walker * pWalker, Select * p)
 				/* This particular expression does not need to be expanded.
 				 */
 				pNew =
-				    sqlite3ExprListAppend(pParse, pNew,
+				    sql_expr_list_append(pParse->db, pNew,
 							  a[k].pExpr);
 				if (pNew) {
 					pNew->a[pNew->nExpr - 1].zName =
@@ -4922,7 +4923,10 @@ selectExpander(Walker * pWalker, Select * p)
 						} else {
 							pExpr = pRight;
 						}
-						pNew = sqlite3ExprListAppend(pParse, pNew, pExpr);
+						pNew =
+							sql_expr_list_append(
+								pParse->db,
+								pNew, pExpr);
 						sqlite3TokenInit(&sColname, zColname);
 						sqlite3ExprListSetName(pParse,
 								       pNew,
@@ -4964,7 +4968,7 @@ selectExpander(Walker * pWalker, Select * p)
 				}
 			}
 		}
-		sqlite3ExprListDelete(db, pEList);
+		sql_expr_list_delete(db, pEList);
 		p->pEList = pNew;
 	}
 #if SQLITE_MAX_COLUMN
@@ -5367,7 +5371,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 		/* If ORDER BY makes no difference in the output then neither does
 		 * DISTINCT so it can be removed too.
 		 */
-		sqlite3ExprListDelete(db, p->pOrderBy);
+		sql_expr_list_delete(db, p->pOrderBy);
 		p->pOrderBy = 0;
 		p->selFlags &= ~SF_Distinct;
 	}
@@ -5614,7 +5618,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 	if ((p->selFlags & (SF_Distinct | SF_Aggregate)) == SF_Distinct
 	    && sqlite3ExprListCompare(sSort.pOrderBy, pEList, -1) == 0) {
 		p->selFlags &= ~SF_Distinct;
-		pGroupBy = p->pGroupBy = sqlite3ExprListDup(db, pEList, 0);
+		pGroupBy = p->pGroupBy = sql_expr_list_dup(db, pEList, 0);
 		/* Notice that even thought SF_Distinct has been cleared from p->selFlags,
 		 * the sDistinct.isTnct is still set.  Hence, isTnct represents the
 		 * original setting of the SF_Distinct flag, not the current setting
@@ -6165,7 +6169,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 
 				if (flag) {
 					pMinMax =
-					    sqlite3ExprListDup(db, pMinMax, 0);
+					    sql_expr_list_dup(db, pMinMax, 0);
 					pDel = pMinMax;
 					assert(db->mallocFailed
 					       || pMinMax != 0);
@@ -6187,7 +6191,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 				    sqlite3WhereBegin(pParse, pTabList, pWhere,
 						      pMinMax, 0, flag, 0);
 				if (pWInfo == 0) {
-					sqlite3ExprListDelete(db, pDel);
+					sql_expr_list_delete(db, pDel);
 					goto select_end;
 				}
 				updateAccumulator(pParse, &sAggInfo);
@@ -6203,7 +6207,7 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 				}
 				sqlite3WhereEnd(pWInfo);
 				finalizeAggFunctions(pParse, &sAggInfo);
-				sqlite3ExprListDelete(db, pDel);
+				sql_expr_list_delete(db, pDel);
 			}
 
 			sSort.pOrderBy = 0;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index a1a5269..5746cb4 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3487,7 +3487,6 @@ void sqlite3PExprAddSelect(Parse *, Expr *, Select *);
 Expr *sqlite3ExprAnd(sqlite3 *, Expr *, Expr *);
 Expr *sqlite3ExprFunction(Parse *, ExprList *, Token *);
 void sqlite3ExprAssignVarNumber(Parse *, Expr *, u32);
-ExprList *sqlite3ExprListAppend(Parse *, ExprList *, Expr *);
 ExprList *sqlite3ExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
 
 /**
@@ -3500,7 +3499,6 @@ void sqlite3ExprListSetSortOrder(ExprList *, enum sort_order sort_order);
 
 void sqlite3ExprListSetName(Parse *, ExprList *, Token *, int);
 void sqlite3ExprListSetSpan(Parse *, ExprList *, ExprSpan *);
-void sqlite3ExprListDelete(sqlite3 *, ExprList *);
 u32 sqlite3ExprListFlags(const ExprList *);
 int sqlite3Init(sqlite3 *);
 int sqlite3InitCallback(void *, int, char **, char **);
@@ -4004,7 +4002,6 @@ void sqlite3MayAbort(Parse *);
 void sqlite3HaltConstraint(Parse *, int, int, char *, i8, u8);
 void sqlite3UniqueConstraint(Parse *, int, Index *);
 Expr *sqlite3ExprDup(sqlite3 *, Expr *, int);
-ExprList *sqlite3ExprListDup(sqlite3 *, ExprList *, int);
 SrcList *sqlite3SrcListDup(sqlite3 *, SrcList *, int);
 IdList *sqlite3IdListDup(sqlite3 *, IdList *);
 Select *sqlite3SelectDup(sqlite3 *, Select *, int);
@@ -4225,7 +4222,6 @@ int sqlite3MatchSpanName(const char *, const char *, const char *);
 int sqlite3ResolveExprNames(NameContext *, Expr *);
 int sqlite3ResolveExprListNames(NameContext *, ExprList *);
 void sqlite3ResolveSelectNames(Parse *, Select *, NameContext *);
-void sqlite3ResolveSelfReference(Parse *, Table *, int, Expr *, ExprList *);
 int sqlite3ResolveOrderGroupBy(Parse *, Select *, ExprList *, const char *);
 
 /**
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 2704816..27902fd 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -53,7 +53,7 @@ sqlite3DeleteTriggerStep(sqlite3 * db, TriggerStep * pTriggerStep)
 		pTriggerStep = pTriggerStep->pNext;
 
 		sql_expr_delete(db, pTmp->pWhere, false);
-		sqlite3ExprListDelete(db, pTmp->pExprList);
+		sql_expr_list_delete(db, pTmp->pExprList);
 		sqlite3SelectDelete(db, pTmp->pSelect);
 		sqlite3IdListDelete(db, pTmp->pIdList);
 
@@ -438,12 +438,12 @@ sqlite3TriggerUpdateStep(sqlite3 * db,	/* The database connection */
 	pTriggerStep = triggerStepAllocate(db, TK_UPDATE, pTableName);
 	if (pTriggerStep) {
 		pTriggerStep->pExprList =
-		    sqlite3ExprListDup(db, pEList, EXPRDUP_REDUCE);
+		    sql_expr_list_dup(db, pEList, EXPRDUP_REDUCE);
 		pTriggerStep->pWhere =
 		    sqlite3ExprDup(db, pWhere, EXPRDUP_REDUCE);
 		pTriggerStep->orconf = orconf;
 	}
-	sqlite3ExprListDelete(db, pEList);
+	sql_expr_list_delete(db, pEList);
 	sql_expr_delete(db, pWhere, false);
 	return pTriggerStep;
 }
@@ -723,7 +723,7 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
 		case TK_UPDATE:{
 				sqlite3Update(pParse,
 					      targetSrcList(pParse, pStep),
-					      sqlite3ExprListDup(db,
+					      sql_expr_list_dup(db,
 								 pStep->
 								 pExprList, 0),
 					      sqlite3ExprDup(db, pStep->pWhere,
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 848ae20..590aad2 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -642,7 +642,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
  update_cleanup:
 	sqlite3DbFree(db, aXRef);	/* Also frees aRegIdx[] and aToOpen[] */
 	sqlite3SrcListDelete(db, pTabList);
-	sqlite3ExprListDelete(db, pChanges);
+	sql_expr_list_delete(db, pChanges);
 	sql_expr_delete(db, pWhere, false);
 	return;
 }
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 8b0f21a..9fb3f18 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -511,11 +511,11 @@ codeEqualityTerm(Parse * pParse,	/* The parsing context */
 							   pExpr, 0);
 
 					pRhs =
-					    sqlite3ExprListAppend(pParse, pRhs,
-								  pNewRhs);
+					    sql_expr_list_append(pParse->db,
+								 pRhs, pNewRhs);
 					pLhs =
-					    sqlite3ExprListAppend(pParse, pLhs,
-								  pNewLhs);
+					    sql_expr_list_append(pParse->db,
+								 pLhs, pNewLhs);
 				}
 			}
 			if (!db->mallocFailed) {
@@ -562,8 +562,8 @@ codeEqualityTerm(Parse * pParse,	/* The parsing context */
 				pLeft->x.pList = pOrigLhs;
 				pX->pLeft = pLeft;
 			}
-			sqlite3ExprListDelete(pParse->db, pLhs);
-			sqlite3ExprListDelete(pParse->db, pRhs);
+			sql_expr_list_delete(pParse->db, pLhs);
+			sql_expr_list_delete(pParse->db, pRhs);
 		}
 
 		if (eType == IN_INDEX_INDEX_DESC) {
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 6da2762..d5a5b07 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -784,8 +784,8 @@ exprAnalyzeOrTerm(SrcList * pSrc,	/* the FROM clause */
 				    sqlite3ExprDup(db, pOrTerm->pExpr->pRight,
 						   0);
 				pList =
-				    sqlite3ExprListAppend(pWInfo->pParse, pList,
-							  pDup);
+					sql_expr_list_append(pWInfo->pParse->db,
+							 pList, pDup);
 				pLeft = pOrTerm->pExpr->pLeft;
 			}
 			assert(pLeft != 0);
@@ -805,7 +805,7 @@ exprAnalyzeOrTerm(SrcList * pSrc,	/* the FROM clause */
 				pTerm = &pWC->a[idxTerm];
 				markTermAsChild(pWC, idxNew, idxTerm);
 			} else {
-				sqlite3ExprListDelete(db, pList);
+				sql_expr_list_delete(db, pList);
 			}
 			pTerm->eOperator = WO_NOOP;	/* case 1 trumps case 3 */
 		}
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> As we would like work with check in server, we need to export
> - sqlite3ExprListAppend as sql_expr_list_append
> - sqlite3ExprListDelete as sql_expr_list_delete
> - sqlite3ExprListDup as sql_expr_list_dup.

I don't understand why we need to use internal parser API calls in
the server. It seems all the server should need to know is how to
invoke the parser with a string fragment, and how to convert the
resulting AST to vdbe, and then how to execute the vdbe/free the
resources. 

Perhaps there should be a method to reset the vdbe for subsequent
execution.

Why do we need to use low-level parser calls in the server?

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Kirill Shcherbatov]

> Why do we need to use low-level parser calls in the server?This functions are required to work with ExprList structure in next commit, 
to duplicate ExprList with checks in space_def_new,  space_def_dup;
to delete them in space_opts_destroy and on checks_array_decode failure; finally, 
to grow checks list width on checks_array_decode iteration.  

I did not want to mix cosmetic and functional changes in one patch.


> I don't understand why we need to use internal parser API calls in
> the server. It seems all the server should need to know is how to
> invoke the parser with a string fragment, and how to convert the
> resulting AST to vdbe, and then how to execute the vdbe/free the
> resources. 
> 
> Perhaps there should be a method to reset the vdbe for subsequent
> execution.
They do not generate Vdbe instructions, it's a service instruments like sql_expr_delete
that already declared in sql.h this way.

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Vladislav Shpilevoy]

Thanks for the patch! See 17 comments below.

On 23/05/2018 17:05, Kirill Shcherbatov wrote:
> As we would like work with check in server, we need to export
> - sqlite3ExprListAppend as sql_expr_list_append
> - sqlite3ExprListDelete as sql_expr_list_delete
> - sqlite3ExprListDup as sql_expr_list_dup.
> 
> Part of #3272.
> ---
>   src/box/sql.h           | 57 ++++++++++++++++++++++++++++++
>   src/box/sql/build.c     | 39 +++++++++++----------
>   src/box/sql/expr.c      | 93 +++++++++++++++++++++----------------------------
>   src/box/sql/fkey.c      | 11 +++---
>   src/box/sql/insert.c    |  2 +-
>   src/box/sql/parse.y     | 81 +++++++++++++++++++++---------------------
>   src/box/sql/prepare.c   |  2 +-
>   src/box/sql/resolve.c   | 37 +++++++-------------
>   src/box/sql/select.c    | 40 +++++++++++----------
>   src/box/sql/sqliteInt.h |  4 ---
>   src/box/sql/trigger.c   |  8 ++---
>   src/box/sql/update.c    |  2 +-
>   src/box/sql/wherecode.c | 12 +++----
>   src/box/sql/whereexpr.c |  6 ++--
>   14 files changed, 214 insertions(+), 180 deletions(-)
> 
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 20f0651..d031d9b 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -178,6 +178,63 @@ int
>   sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
>   
>   /**
> + * Duplicate Expr list.
> + * The flags parameter contains a combination of the EXPRDUP_XXX flags.
> + * If the EXPRDUP_REDUCE flag is set, then the structure returned is a
> + * truncated version of the usual Expr structure that will be stored as
> + * part of the in-memory representation of the database schema.
> + * @param db The database connection.
> + * @param p The ExprList to duplicate.
> + * @param flags EXPRDUP_XXX flags.
> + * @retval NULL on memory allocation error.
> + * @retval not NULL on success.
> + */
> +struct ExprList *
> +sql_expr_list_dup(struct sqlite3 * db, struct ExprList * p, int flags);

1. Please, do not put extra white space after *.

> +
> +/**
> + * Add a new element to the end of an expression list.  If expr_list is
> + * 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);

2. Invalid indentation.

> +
> +/**
> + * Resolve names in expressions that can only reference a single table:
> + * *   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.

3. No pParse.

> + * @param parser Parsing context.
> + * @param table The table being referenced.
> + * @param type NC_IsCheck or NC_PartIdx or NC_IdxExpr.
> + * @param expr Expression to resolve.  May be NULL.
> + * @param expr_list Expression list to resolve.  May be NUL.
> + */
> +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 afb1128..d00bb1d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2952,7 +2952,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>   		Token prevCol;
>   		sqlite3TokenInit(&prevCol, pTab->def->fields[
>   			pTab->def->field_count - 1].name);
> -		col_list = sqlite3ExprListAppend(parse, 0,
> +		col_list = sql_expr_list_append(parse->db, NULL,
>   						 sqlite3ExprAlloc(db, TK_ID,

4. Wrong indentation.

>   								  &prevCol, 0));
>   		if (col_list == NULL)
> @@ -3022,8 +3023,8 @@ sql_create_index(struct Parse *parse, struct Token *token,
>   	for (i = 0, col_listItem = col_list->a; i < col_list->nExpr;
>   	     i++, col_listItem++) {
>   		Expr *pCExpr;	/* The i-th index expression */
> -		sqlite3ResolveSelfReference(parse, pTab, NC_IdxExpr,
> -					    col_listItem->pExpr, 0);
> +		sql_resolve_self_reference(parse, pTab, NC_IdxExpr,
> +					    col_listItem->pExpr, NULL);

5. Same.

>   		if (parse->nErr > 0)
>   			goto exit_create_index;
>   		pCExpr = sqlite3ExprSkipCollate(col_listItem->pExpr);
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 64991ee..ecb0915 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -1444,8 +1443,8 @@ sqlite3ExprDup(sqlite3 * db, Expr * p, int flags)
>   	return p ? sql_expr_dup(db, p, flags, 0) : 0;
>   }
>   
> -ExprList *
> -sqlite3ExprListDup(sqlite3 * db, ExprList * p, int flags)
> +struct ExprList *
> +sql_expr_list_dup(struct sqlite3 * db, struct ExprList *p, int flags)

6. Please, consider this diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index ecb091554..fa43569a7 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1446,38 +1446,37 @@ sqlite3ExprDup(sqlite3 * db, Expr * p, int flags)
  struct ExprList *
  sql_expr_list_dup(struct sqlite3 * db, struct ExprList *p, int flags)
  {
-	ExprList *pNew;
  	struct ExprList_item *pItem, *pOldItem;
  	int i;
-	Expr *pPriorSelectCol = 0;
-	assert(db != 0);
-	if (p == 0)
-		return 0;
-	pNew = sqlite3DbMallocRawNN(db, sizeof(*pNew));
-	if (pNew == 0)
-		return 0;
+	Expr *pPriorSelectCol = NULL;
+	assert(db != NULL);
+	if (p == NULL)
+		return NULL;
+	ExprList *pNew = sqlite3DbMallocRawNN(db, sizeof(*pNew));
+	if (pNew == NULL)
+		return NULL;
  	pNew->nExpr = i = p->nExpr;
-	if ((flags & EXPRDUP_REDUCE) == 0)
+	if ((flags & EXPRDUP_REDUCE) == 0) {
  		for (i = 1; i < p->nExpr; i += i) {
  		}
+	}
  	pNew->a = pItem = sqlite3DbMallocRawNN(db, i * sizeof(p->a[0]));
-	if (pItem == 0) {
+	if (pItem == NULL) {
  		sqlite3DbFree(db, pNew);
-		return 0;
+		return NULL;
  	}
  	pOldItem = p->a;
  	for (i = 0; i < p->nExpr; i++, pItem++, pOldItem++) {
  		Expr *pOldExpr = pOldItem->pExpr;
  		Expr *pNewExpr;
  		pItem->pExpr = sqlite3ExprDup(db, pOldExpr, flags);
-		if (pOldExpr
-		    && pOldExpr->op == TK_SELECT_COLUMN
-		    && (pNewExpr = pItem->pExpr) != 0) {
+		if (pOldExpr != NULL && pOldExpr->op == TK_SELECT_COLUMN
+		    && (pNewExpr = pItem->pExpr) != NULL) {
  			assert(pNewExpr->iColumn == 0 || i > 0);
  			if (pNewExpr->iColumn == 0) {
  				assert(pOldExpr->pLeft == pOldExpr->pRight);
  				pPriorSelectCol = pNewExpr->pLeft =
-				    pNewExpr->pRight;
+					pNewExpr->pRight;
  			} else {
  				assert(i > 0);
  				assert(pItem[-1].pExpr != 0);


>   {
>   	ExprList *pNew;
>   	struct ExprList_item *pItem, *pOldItem;
> @@ -1618,51 +1617,39 @@ sqlite3SelectDup(sqlite3 * db, Select * p, int flags)
>   	return pNew;
>   }
>   
> -/*
> - * Add a new element to the end of an expression list.  If pList is
> - * 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.
> - */
> -ExprList *
> -sqlite3ExprListAppend(Parse * pParse,	/* Parsing context */
> -		      ExprList * pList,	/* List to which to append. Might be NULL */
> -		      Expr * pExpr	/* Expression to be appended. Might be NULL */
> -    )
> +struct ExprList *
> +sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
> +	struct Expr *expr)

7.

@@ -1618,8 +1617,8 @@ sqlite3SelectDup(sqlite3 * db, Select * p, int flags)
  }
  
  struct ExprList *
-sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
-       struct Expr *expr)
+sql_expr_list_append(struct sqlite3 *db, struct ExprList *expr_list,
+                    struct Expr *expr)
  {
         assert(db != NULL);
         if (expr_list == NULL) {
@@ -1650,7 +1649,7 @@ sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
         /* Avoid leaking memory if malloc has failed. */
         sql_expr_delete(db, expr, false);
         sql_expr_list_delete(db, expr_list);
-       return 0;
+       return NULL;
  }
> @@ -1847,10 +1834,10 @@ exprListDeleteNN(sqlite3 * db, ExprList * pList)
>   }
>   
>   void
> -sqlite3ExprListDelete(sqlite3 * db, ExprList * pList)
> +sql_expr_list_delete(sqlite3 *db, ExprList *expr_list)
>   {
> -	if (pList)
> -		exprListDeleteNN(db, pList);
> +	if (expr_list)
> +		exprListDeleteNN(db, expr_list);
>   }

8.

@@ -1836,7 +1836,7 @@ exprListDeleteNN(sqlite3 * db, ExprList * pList)
  void
  sql_expr_list_delete(sqlite3 *db, ExprList *expr_list)
  {
-       if (expr_list)
+       if (expr_list != NULL)
                 exprListDeleteNN(db, expr_list);
  }
> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
> index 2ab8fdd..3e30a44 100644
> --- a/src/box/sql/fkey.c
> +++ b/src/box/sql/fkey.c
> @@ -1376,7 +1377,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
>   				pRaise->affinity = ON_CONFLICT_ACTION_ABORT;
>   			}
>   			pSelect = sqlite3SelectNew(pParse,
> -						   sqlite3ExprListAppend(pParse,
> +						   sql_expr_list_append(pParse->db,
>   									 0,
>   									 pRaise),
>   						   sqlite3SrcListAppend(db, 0,

9.

@@ -1378,8 +1378,8 @@ fkActionTrigger(Parse * pParse,   /* Parse context */
                         }
                         pSelect = sqlite3SelectNew(pParse,
                                                    sql_expr_list_append(pParse->db,
-                                                                        0,
-                                                                        pRaise),
+                                                                       NULL,
+                                                                       pRaise),
                                                    sqlite3SrcListAppend(db, 0,
                                                                         &tFrom),
                                                    pWhere, 0, 0, 0, 0, 0, 0);
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 7f0858a..fd82119 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -1578,40 +1578,27 @@ sqlite3ResolveSelectNames(Parse * pParse,	/* The parser context */
>   void
> -sqlite3ResolveSelfReference(Parse * pParse,	/* Parsing context */
> -			    Table * pTab,	/* The table being referenced */
> -			    int type,	/* NC_IsCheck or NC_PartIdx or NC_IdxExpr */
> -			    Expr * pExpr,	/* Expression to resolve.  May be NULL. */
> -			    ExprList * pList	/* Expression list to resolve.  May be NUL. */
> -    )
> +sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
> +			   struct Expr *expr, struct ExprList *expr_list)

10.

@@ -1582,9 +1582,9 @@ void
  sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
                            struct Expr *expr, struct ExprList *expr_list)
  {
-       /* Fake SrcList for pParse->pNewTable */
+       /* Fake SrcList for parser->pNewTable */
         SrcList sSrc;
-       /* Name context for pParse->pNewTable */
+       /* Name context for parser->pNewTable */
         NameContext sNC;
  
         assert(type == NC_IsCheck || type == NC_PartIdx || type == NC_IdxExpr);
@@ -1597,7 +1597,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
         sNC.pParse = parser;
         sNC.pSrcList = &sSrc;
         sNC.ncFlags = type;
-       if (sqlite3ResolveExprNames(&sNC, expr))
+       if (sqlite3ResolveExprNames(&sNC, expr) != 0)
                 return;
         if (expr_list != NULL)
                 sqlite3ResolveExprListNames(&sNC, expr_list);
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 75bd53d..cd305be 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -151,7 +151,7 @@ sqlite3SelectNew(Parse * pParse,	/* Parsing context */
>   	}
>   	if (pEList == 0) {
>   		pEList =
> -		    sqlite3ExprListAppend(pParse, 0,
> +		    sql_expr_list_append(pParse->db, NULL,
>   					  sqlite3Expr(db, TK_ASTERISK, 0));

11.

@@ -150,9 +150,8 @@ sqlite3SelectNew(Parse * pParse,    /* Parsing context */
                 pNew = &standin;
         }
         if (pEList == 0) {
-               pEList =
-                   sql_expr_list_append(pParse->db, NULL,
-                                         sqlite3Expr(db, TK_ASTERISK, 0));
+               pEList = sql_expr_list_append(pParse->db, NULL,
+                                             sqlite3Expr(db, TK_ASTERISK, 0));
         }
         struct session MAYBE_UNUSED *user_session;
         user_session = current_session();

>   	}
>   	struct session MAYBE_UNUSED *user_session;
> @@ -3191,7 +3191,7 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
>   				pNew->flags |= EP_IntValue;
>   				pNew->u.iValue = i;
>   				pOrderBy =
> -				    sqlite3ExprListAppend(pParse, pOrderBy,
> +				    sql_expr_list_append(pParse->db, pOrderBy,
>   							  pNew);

12.

@@ -3190,9 +3189,8 @@ multiSelectOrderBy(Parse * pParse,        /* Parsing context */
                                         return SQLITE_NOMEM_BKPT;
                                 pNew->flags |= EP_IntValue;
                                 pNew->u.iValue = i;
-                               pOrderBy =
-                                   sql_expr_list_append(pParse->db, pOrderBy,
-                                                         pNew);
+                               pOrderBy = sql_expr_list_append(pParse->db,
+                                                               pOrderBy, pNew);
                                 if (pOrderBy)
                                         pOrderBy->a[nOrderBy++].u.x.
                                             iOrderByCol = (u16) i;
> @@ -4376,7 +4376,8 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p)
>   	*pNew = *p;
>   	p->pSrc = pNewSrc;
>   	p->pEList =
> -	    sqlite3ExprListAppend(pParse, 0, sqlite3Expr(db, TK_ASTERISK, 0));
> +	    sql_expr_list_append(pParse->db, NULL,
> +				 sqlite3Expr(db, TK_ASTERISK, 0));

13.

@@ -4375,9 +4373,8 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p)
                 return WRC_Abort;
         *pNew = *p;
         p->pSrc = pNewSrc;
-       p->pEList =
-           sql_expr_list_append(pParse->db, NULL,
-                                sqlite3Expr(db, TK_ASTERISK, 0));
+       p->pEList = sql_expr_list_append(pParse->db, NULL,
+                                        sqlite3Expr(db, TK_ASTERISK, 0));
         p->op = TK_SELECT;
         p->pWhere = 0;
         pNew->pGroupBy = 0;
> @@ -4817,7 +4818,7 @@ selectExpander(Walker * pWalker, Select * p)
>   				/* This particular expression does not need to be expanded.
>   				 */
>   				pNew =
> -				    sqlite3ExprListAppend(pParse, pNew,
> +				    sql_expr_list_append(pParse->db, pNew,
>   							  a[k].pExpr);

14.

@@ -4817,9 +4817,8 @@ selectExpander(Walker * pWalker, Select * p)
                             ) {
                                 /* This particular expression does not need to be expanded.
                                  */
-                               pNew =
-                                   sql_expr_list_append(pParse->db, pNew,
-                                                         a[k].pExpr);
+                               pNew = sql_expr_list_append(pParse->db, pNew,
+                                                           a[k].pExpr);
                                 if (pNew) {
                                         pNew->a[pNew->nExpr - 1].zName =
                                             a[k].zName;

> @@ -4922,7 +4923,10 @@ selectExpander(Walker * pWalker, Select * p)
>   						} else {
>   							pExpr = pRight;
>   						}
> -						pNew = sqlite3ExprListAppend(pParse, pNew, pExpr);
> +						pNew =
> +							sql_expr_list_append(
> +								pParse->db,
> +								pNew, pExpr);

15.

@@ -4923,10 +4922,8 @@ selectExpander(Walker * pWalker, Select * p)
                                                 } else {
                                                         pExpr = pRight;
                                                 }
-                                               pNew =
-                                                       sql_expr_list_append(
-                                                               pParse->db,
-                                                               pNew, pExpr);
+                                               pNew = sql_expr_list_append(
+                                                       pParse->db,pNew, pExpr);
                                                 sqlite3TokenInit(&sColname, zColname);
                                                 sqlite3ExprListSetName(pParse,
                                                                        pNew,

> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 2704816..27902fd 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -723,7 +723,7 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
>   		case TK_UPDATE:{
>   				sqlite3Update(pParse,
>   					      targetSrcList(pParse, pStep),
> -					      sqlite3ExprListDup(db,
> +					      sql_expr_list_dup(db,
>   								 pStep->
>   								 pExprList, 0),

16.

@@ -724,8 +724,8 @@ codeTriggerProgram(Parse * pParse,  /* The parser context */
                                 sqlite3Update(pParse,
                                               targetSrcList(pParse, pStep),
                                               sql_expr_list_dup(db,
-                                                                pStep->
-                                                                pExprList, 0),
+                                                               pStep->pExprList,
+                                                               0),
                                               sqlite3ExprDup(db, pStep->pWhere,
                                                              0),
                                               pParse->eOrconf);

> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> index 6da2762..d5a5b07 100644
> --- a/src/box/sql/whereexpr.c
> +++ b/src/box/sql/whereexpr.c
> @@ -784,8 +784,8 @@ exprAnalyzeOrTerm(SrcList * pSrc,	/* the FROM clause */
>   				    sqlite3ExprDup(db, pOrTerm->pExpr->pRight,
>   						   0);
>   				pList =
> -				    sqlite3ExprListAppend(pWInfo->pParse, pList,
> -							  pDup);
> +					sql_expr_list_append(pWInfo->pParse->db,
> +							 pList, pDup);

17.

@@ -783,9 +783,8 @@ exprAnalyzeOrTerm(SrcList * pSrc,   /* the FROM clause */
                                 pDup =
                                     sqlite3ExprDup(db, pOrTerm->pExpr->pRight,
                                                    0);
-                               pList =
-                                       sql_expr_list_append(pWInfo->pParse->db,
-                                                        pList, pDup);
+                               pList = sql_expr_list_append(pWInfo->pParse->db,
+                                                            pList, pDup);
                                 pLeft = pOrTerm->pExpr->pLeft;

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Kirill Shcherbatov]

> 1. Please, do not put extra white space after *.
> 2. Invalid indentation.
> 3. No pParse.
> 4. Wrong indentation.
> 5. Same.
> 6. Please, consider this diff:

diff --git a/src/box/sql.h b/src/box/sql.h
index ec4f298..2a87d14 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -190,7 +190,7 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
  * @retval not NULL on success.
  */
 struct ExprList *
-sql_expr_list_dup(struct sqlite3 * db, struct ExprList * p, int flags);
+sql_expr_list_dup(struct sqlite3 *db, struct ExprList * p, int flags);
 
 /**
  * Free AST pointed by expr list.
@@ -214,8 +214,8 @@ sql_expr_list_delete(struct sqlite3 *db, struct ExprList *expr_list);
  * @retval not NULL on success.
  */
 struct ExprList *
-sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
-	struct Expr *expr);
+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:
@@ -223,7 +223,7 @@ sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
  * *   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.
+ * Any errors cause an error message to be set in parser.
  * @param parser Parsing context.
  * @param table The table being referenced.
  * @param type NC_IsCheck or NC_PartIdx or NC_IdxExpr.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 674f980..a30923c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2949,11 +2949,13 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	 */
 	if (col_list == NULL) {
 		Token prevCol;
-		sqlite3TokenInit(&prevCol, pTab->def->fields[
-			pTab->def->field_count - 1].name);
-		col_list = sql_expr_list_append(parse->db, NULL,
-						 sqlite3ExprAlloc(db, TK_ID,
-								  &prevCol, 0));
+		sqlite3TokenInit(&prevCol,
+				 pTab->def->fields[
+				 	pTab->def->field_count - 1].name);
+		col_list =
+			sql_expr_list_append(parse->db, NULL,
+					     sqlite3ExprAlloc(db, TK_ID,
+							      &prevCol, 0));
 		if (col_list == NULL)
 			goto exit_create_index;
 		assert(col_list->nExpr == 1);
@@ -3023,7 +3025,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 	     i++, col_listItem++) {
 		Expr *pCExpr;	/* The i-th index expression */
 		sql_resolve_self_reference(parse, pTab, NC_IdxExpr,
-					    col_listItem->pExpr, NULL);
+					   col_listItem->pExpr, NULL);
 		if (parse->nErr > 0)
 			goto exit_create_index;
 		pCExpr = sqlite3ExprSkipCollate(col_listItem->pExpr);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 4771b69..f11e84a 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1446,38 +1446,37 @@ sqlite3ExprDup(sqlite3 * db, Expr * p, int flags)
 struct ExprList *
 sql_expr_list_dup(struct sqlite3 * db, struct ExprList *p, int flags)
 {
-	ExprList *pNew;
 	struct ExprList_item *pItem, *pOldItem;
 	int i;
-	Expr *pPriorSelectCol = 0;
-	assert(db != 0);
-	if (p == 0)
-		return 0;
-	pNew = sqlite3DbMallocRawNN(db, sizeof(*pNew));
-	if (pNew == 0)
-		return 0;
+	Expr *pPriorSelectCol = NULL;
+	assert(db != NULL);
+	if (p == NULL)
+		return NULL;
+	ExprList *pNew = sqlite3DbMallocRawNN(db, sizeof(*pNew));
+	if (pNew == NULL)
+		return NULL;
 	pNew->nExpr = i = p->nExpr;
-	if ((flags & EXPRDUP_REDUCE) == 0)
+	if ((flags & EXPRDUP_REDUCE) == 0) {
 		for (i = 1; i < p->nExpr; i += i) {
 		}
+	}
 	pNew->a = pItem = sqlite3DbMallocRawNN(db, i * sizeof(p->a[0]));
-	if (pItem == 0) {
+	if (pItem == NULL) {
 		sqlite3DbFree(db, pNew);
-		return 0;
+		return NULL;
 	}
 	pOldItem = p->a;
 	for (i = 0; i < p->nExpr; i++, pItem++, pOldItem++) {
 		Expr *pOldExpr = pOldItem->pExpr;
 		Expr *pNewExpr;
 		pItem->pExpr = sqlite3ExprDup(db, pOldExpr, flags);
-		if (pOldExpr
-		    && pOldExpr->op == TK_SELECT_COLUMN
-		    && (pNewExpr = pItem->pExpr) != 0) {
+		if (pOldExpr != NULL && pOldExpr->op == TK_SELECT_COLUMN &&
+		    (pNewExpr = pItem->pExpr) != NULL) {
 			assert(pNewExpr->iColumn == 0 || i > 0);
 			if (pNewExpr->iColumn == 0) {
 				assert(pOldExpr->pLeft == pOldExpr->pRight);
 				pPriorSelectCol = pNewExpr->pLeft =
-				    pNewExpr->pRight;
+					pNewExpr->pRight;
 			} else {
 				assert(i > 0);
 				assert(pItem[-1].pExpr != 0);
@@ -1619,7 +1618,7 @@ sqlite3SelectDup(sqlite3 * db, Select * p, int flags)
 
 struct ExprList *
 sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
-	struct Expr *expr)
+		     struct Expr *expr)
 {
 	assert(db != NULL);
 	if (expr_list == NULL) {
@@ -1650,7 +1649,7 @@ sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
 	/* Avoid leaking memory if malloc has failed. */
 	sql_expr_delete(db, expr, false);
 	sql_expr_list_delete(db, expr_list);
-	return 0;
+	return NULL;
 }
 
 /*
@@ -1836,7 +1835,7 @@ exprListDeleteNN(sqlite3 * db, ExprList * pList)
 void
 sql_expr_list_delete(sqlite3 *db, ExprList *expr_list)
 {
-	if (expr_list)
+	if (expr_list != NULL)
 		exprListDeleteNN(db, expr_list);
 }
 
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 3e30a44..ae3cc12 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1378,8 +1378,8 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 			}
 			pSelect = sqlite3SelectNew(pParse,
 						   sql_expr_list_append(pParse->db,
-									 0,
-									 pRaise),
+									NULL,
+									pRaise),
 						   sqlite3SrcListAppend(db, 0,
 									&tFrom),
 						   pWhere, 0, 0, 0, 0, 0, 0);
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index fd82119..53b6c66 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -1582,9 +1582,9 @@ void
 sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
 			   struct Expr *expr, struct ExprList *expr_list)
 {
-	/* Fake SrcList for pParse->pNewTable */
+	/* Fake SrcList for parser->pNewTable */
 	SrcList sSrc;
-	/* Name context for pParse->pNewTable */
+	/* Name context for parser->pNewTable */
 	NameContext sNC;
 
 	assert(type == NC_IsCheck || type == NC_PartIdx || type == NC_IdxExpr);
@@ -1597,7 +1597,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
 	sNC.pParse = parser;
 	sNC.pSrcList = &sSrc;
 	sNC.ncFlags = type;
-	if (sqlite3ResolveExprNames(&sNC, expr))
+	if (sqlite3ResolveExprNames(&sNC, expr) != 0)
 		return;
 	if (expr_list != NULL)
 		sqlite3ResolveExprListNames(&sNC, expr_list);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 41f0bf4..c8b37c6 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -150,9 +150,8 @@ sqlite3SelectNew(Parse * pParse,	/* Parsing context */
 		pNew = &standin;
 	}
 	if (pEList == 0) {
-		pEList =
-		    sql_expr_list_append(pParse->db, NULL,
-					  sqlite3Expr(db, TK_ASTERISK, 0));
+		pEList = sql_expr_list_append(pParse->db, NULL,
+					      sqlite3Expr(db, TK_ASTERISK, 0));
 	}
 	struct session MAYBE_UNUSED *user_session;
 	user_session = current_session();
@@ -3190,9 +3189,8 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
 					return SQLITE_NOMEM_BKPT;
 				pNew->flags |= EP_IntValue;
 				pNew->u.iValue = i;
-				pOrderBy =
-				    sql_expr_list_append(pParse->db, pOrderBy,
-							  pNew);
+				pOrderBy = sql_expr_list_append(pParse->db,
+								pOrderBy, pNew);
 				if (pOrderBy)
 					pOrderBy->a[nOrderBy++].u.x.
 					    iOrderByCol = (u16) i;
@@ -4375,9 +4373,8 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p)
 		return WRC_Abort;
 	*pNew = *p;
 	p->pSrc = pNewSrc;
-	p->pEList =
-	    sql_expr_list_append(pParse->db, NULL,
-				 sqlite3Expr(db, TK_ASTERISK, 0));
+	p->pEList = sql_expr_list_append(pParse->db, NULL,
+					 sqlite3Expr(db, TK_ASTERISK, 0));
 	p->op = TK_SELECT;
 	p->pWhere = 0;
 	pNew->pGroupBy = 0;
@@ -4817,10 +4814,9 @@ selectExpander(Walker * pWalker, Select * p)
 			    ) {
 				/* This particular expression does not need to be expanded.
 				 */
-				pNew =
-				    sql_expr_list_append(pParse->db, pNew,
-							  a[k].pExpr);
-				if (pNew) {
+				pNew = sql_expr_list_append(pParse->db, pNew,
+							    a[k].pExpr);
+				if (pNew != NULL) {
 					pNew->a[pNew->nExpr - 1].zName =
 					    a[k].zName;
 					pNew->a[pNew->nExpr - 1].zSpan =
@@ -4923,16 +4919,14 @@ selectExpander(Walker * pWalker, Select * p)
 						} else {
 							pExpr = pRight;
 						}
-						pNew =
-							sql_expr_list_append(
-								pParse->db,
-								pNew, pExpr);
+						pNew = sql_expr_list_append(
+							pParse->db, pNew, pExpr);
 						sqlite3TokenInit(&sColname, zColname);
 						sqlite3ExprListSetName(pParse,
 								       pNew,
 								       &sColname,
 								       0);
-						if (pNew
+						if (pNew != NULL
 						    && (p->
 							selFlags &
 							SF_NestedFrom) != 0) {
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 27902fd..ea35211 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -724,8 +724,8 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
 				sqlite3Update(pParse,
 					      targetSrcList(pParse, pStep),
 					      sql_expr_list_dup(db,
-								 pStep->
-								 pExprList, 0),
+								pStep->pExprList,
+								0),
 					      sqlite3ExprDup(db, pStep->pWhere,
 							     0),
 					      pParse->eOrconf);
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index d5a5b07..d1eca43 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -783,9 +783,8 @@ exprAnalyzeOrTerm(SrcList * pSrc,	/* the FROM clause */
 				pDup =
 				    sqlite3ExprDup(db, pOrTerm->pExpr->pRight,
 						   0);
-				pList =
-					sql_expr_list_append(pWInfo->pParse->db,
-							 pList, pDup);
+				pList = sql_expr_list_append(pWInfo->pParse->db,
+							     pList, pDup);
 				pLeft = pOrTerm->pExpr->pLeft;
 			}
 			assert(pLeft != 0);

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Vladislav Shpilevoy]

Thanks for the fixes! See my 3 comments below.

On 25/05/2018 14:53, Kirill Shcherbatov wrote:
>> 1. Please, do not put extra white space after *.
>> 2. Invalid indentation.
>> 3. No pParse.
>> 4. Wrong indentation.
>> 5. Same.
>> 6. Please, consider this diff:

1. Please, do not decontextualize my comments. It is hard
to restore for which code they were intended.

> 
> diff --git a/src/box/sql.h b/src/box/sql.h
> index ec4f298..2a87d14 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -190,7 +190,7 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
>    * @retval not NULL on success.
>    */
>   struct ExprList *
> -sql_expr_list_dup(struct sqlite3 * db, struct ExprList * p, int flags);
> +sql_expr_list_dup(struct sqlite3 *db, struct ExprList * p, int flags);

> 1. Please, do not put extra white space after *.

2. Still not fixed.

3. Why did you ignore part of comment 7 from the previous review?

[tarantool-patches] Re: [PATCH v7 6/7] sql: export funcs defined on Expr, ExprList to sql.h

[Kirill Shcherbatov]

>> 1. Please, do not put extra white space after *.
> 2. Still not fixed.
diff --git a/src/box/sql.h b/src/box/sql.h
index a02d156..c4d76a7 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -191,7 +191,7 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *table);
  * @retval not NULL on success.
  */
 struct ExprList *
-sql_expr_list_dup(struct sqlite3 *db, struct ExprList * p, int flags);
+sql_expr_list_dup(struct sqlite3 *db, struct ExprList *p, int flags);
 
 /**
  * Free AST pointed by expr list.
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index cc3278b..d8c6c87 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1447,7 +1447,7 @@ sqlite3ExprDup(sqlite3 * db, Expr * p, int flags)
 }
 
 struct ExprList *
-sql_expr_list_dup(struct sqlite3 * db, struct ExprList *p, int flags)
+sql_expr_list_dup(struct sqlite3 *db, struct ExprList *p, int flags)
 {
        struct ExprList_item *pItem, *pOldItem;
        int i;

> 3. Why did you ignore part of comment 7 from the previous review?
-sql_expr_list_append(struct sqlite3 * db, struct ExprList *expr_list,
+sql_expr_list_append(struct sqlite3 *db, struct ExprList *expr_list,
                     struct Expr *expr)

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

[Kirill Shcherbatov]

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()));
+	}
 	auto def_guard = make_scoped_guard([=] { space_def_delete(def); });
 	struct engine *engine = engine_find_xc(def->engine_name);
 	engine_check_space_def_xc(engine, def);
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 } }
 
 #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} }
 
 #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"}, ..].
+ * @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);
 
 const struct space_opts space_opts_default = {
 	/* .temporary = */ false,
 	/* .view = */ false,
 	/* .sql        = */ NULL,
+	/* .checks     = */ NULL,
 };
 
 const struct opt_def space_opts_reg[] = {
 	OPT_DEF("temporary", OPT_BOOL, struct space_opts, temporary),
 	OPT_DEF("view", OPT_BOOL, struct space_opts, is_view),
 	OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql),
+	OPT_DEF_ARRAY("checks", struct space_opts, checks,
+		checks_array_decode),
 	OPT_END,
 };
 
@@ -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);
+	}
 	return def;
 }
 
@@ -231,3 +272,58 @@ space_def_delete(struct space_def *def)
 	TRASH(def);
 	free(def);
 }
+
+void
+space_opts_destroy(struct space_opts *opts)
+{
+	free(opts->sql);
+	sql_expr_list_delete(sql_get(), opts->checks);
+	TRASH(opts);
+}
+
+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");
+				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;
+			}
+		}
+		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/space_def.h b/src/box/space_def.h
index 5328203..e9bd283 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -61,6 +61,10 @@ struct space_opts {
 	 * SQL statement that produced this space.
 	 */
 	char *sql;
+	/**
+	* SQL Checks expressions list.
+	*/
+	struct ExprList *checks;
 };
 
 extern const struct space_opts space_opts_default;
@@ -79,12 +83,8 @@ space_opts_create(struct space_opts *opts)
 /**
  * Destroy space options
  */
-static inline void
-space_opts_destroy(struct space_opts *opts)
-{
-	free(opts->sql);
-	TRASH(opts);
-}
+void
+space_opts_destroy(struct space_opts *opts);
 
 /** Space metadata. */
 struct space_def {
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
@@ -1508,13 +1508,49 @@ 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);
+	int checks_cnt = has_checks ? pTable->def->opts.checks->nExpr : 0;
+
+	int map_fields = 1;
+	map_fields += is_view == true;
+	map_fields += (checks_cnt > 0);
+	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 (checks_cnt == 0)
+		return (int)(p - base);
+	/* Encode checks. */
+	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;
+		p = enc->encode_map(p, items);
+		/*
+		 * a[i].pExpr could be NULL for VIEW column names
+		 * represented as checks.
+		 */
+		if (a[i].pExpr != NULL) {
+			Expr *pExpr = a[i].pExpr;
+			assert(pExpr->u.zToken != NULL);
+			p = enc->encode_str(p, "expr_str", 8);
+			p = enc->encode_str(p, pExpr->u.zToken,
+					    strlen(pExpr->u.zToken));
+		}
+		if (a[i].zName != NULL) {
+			p = enc->encode_str(p, "name", 10);
+			p = enc->encode_str(p, a[i].zName,
+					    strlen(a[i].zName));
+		}
+	}
 	return (int)(p - base);
 }
 
@@ -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);
+		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;
+}
+
+static int
+update_space_def_callback(Walker *pWalker, Expr *pExpr)
+{
+	if (pExpr->op == TK_COLUMN && ExprHasProperty(pExpr, EP_Resolved))
+		pExpr->space_def = (struct space_def *) pWalker->pParse;
+	return WRC_Continue;
+}
+
+void
+sql_checks_update_space_def_reference(ExprList *expr_list,
+				      struct space_def *def)
+{
+	assert(expr_list != NULL);
+	Walker w;
+	memset(&w, 0, sizeof(w));
+	w.xExprCallback = update_space_def_callback;
+	w.pParse = (void *)def;
+
+	for (int i = 0; i < expr_list->nExpr; i++)
+		sqlite3WalkExpr(&w, expr_list->a[i].pExpr);
+}
+
+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;
+
+	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.h b/src/box/sql.h
index d031d9b..e626f76 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -103,6 +103,15 @@ struct Expr*
 space_column_default_expr(uint32_t space_id, uint32_t fieldno);
 
 /**
+ * Get server checks list by space_id.
+ * @param space_id Space ID.
+ * @retval NULL on error.
+ * @param not NULL on success.
+ */
+struct ExprList *
+space_checks_expr_list(uint32_t space_id);
+
+/**
  * Return the number of bytes required to create a duplicate of the
  * expression passed as the first argument. The second argument is a
  * mask containing EXPRDUP_XXX flags.
@@ -235,6 +244,40 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
 			   struct Expr *expr, struct ExprList *expr_list);
 
 /**
+ * Initialize check_list_item.
+ * @param expr_list ExprList with item.
+ * @param idx item index.
+ * @param expr_name expression name (optional).
+ * @param expr_name_len expresson name length (optional).
+ * @param expr_str expression to build string.
+ * @param expr_str_len expression to build string length.
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+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);
+
+/**
+ * Resolve space_def references checks for expr_list.
+ * @param expr_list to modify.
+ * @param def to refer to.
+ */
+int
+sql_checks_resolve_space_def_reference(struct ExprList *expr_list,
+				       struct space_def *def);
+
+/**
+ * Update space_def references for expr_list.
+ * @param expr_list to modify.
+ * @param def to refer to.
+ */
+void
+sql_checks_update_space_def_reference(struct ExprList *expr_list,
+					struct space_def *def);
+
+/**
  * Initialize a new parser object.
  * A number of service allocations are performed on the region, which is also
  * cleared in the destroy function.
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
@@ -406,7 +406,6 @@ deleteTable(sqlite3 * db, Table * pTable)
 	sqlite3DbFree(db, pTable->aCol);
 	sqlite3DbFree(db, pTable->zColAff);
 	sqlite3SelectDelete(db, pTable->pSelect);
-	sql_expr_list_delete(db, pTable->pCheck);
 	assert(pTable->def != NULL);
 	/* Do not delete pTable->def allocated on region. */
 	if (!pTable->def->opts.temporary)
@@ -1022,19 +1021,29 @@ sqlite3AddPrimaryKey(Parse * pParse,	/* Parsing context */
 void
 sql_add_check_constraint(Parse *parser, ExprSpan *span)
 {
-#ifndef SQLITE_OMIT_CHECK
 	struct Expr *expr = span->pExpr;
 	Table *table = parser->pNewTable;
 	if (table != NULL) {
-		table->pCheck =
-			sql_expr_list_append(parser->db, table->pCheck, expr);
+		expr->u.zToken =
+			sqlite3DbStrNDup(parser->db, (char *)span->zStart,
+					 (int)(span->zEnd - span->zStart));
+		if (expr->u.zToken == NULL)
+			goto release_expr;
+		table->def->opts.checks =
+			sql_expr_list_append(parser->db,
+					     table->def->opts.checks, expr);
+		if (table->def->opts.checks == NULL) {
+			sqlite3DbFree(parser->db, expr->u.zToken);
+			goto release_expr;
+		}
 		if (parser->constraintName.n) {
-			sqlite3ExprListSetName(parser, table->pCheck,
+			sqlite3ExprListSetName(parser, table->def->opts.checks,
 					       &parser->constraintName, 1);
 		}
-	} else
-#endif
+	} else {
+release_expr:
 		sql_expr_delete(parser->db, expr, false);
+	}
 }
 
 /*
@@ -1177,6 +1186,16 @@ space_is_view(Table *table) {
 	return space->def->opts.is_view;
 }
 
+struct ExprList *
+space_checks_expr_list(uint32_t space_id)
+{
+	struct space *space;
+	space = space_by_id(space_id);
+	assert(space != NULL);
+	assert(space->def != NULL);
+	return space->def->opts.checks;
+}
+
 /**
  * Create cursor which will be positioned to the space/index.
  * It makes space lookup and loads pointer to it into register,
@@ -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;
-
-#ifndef SQLITE_OMIT_CHECK
-	/* Resolve names in all CHECK constraint expressions.
-	 */
-	if (p->pCheck) {
-		sql_resolve_self_reference(pParse, p, NC_IsCheck, NULL,
-					    p->pCheck);
-	}
-#endif				/* !defined(SQLITE_OMIT_CHECK) */
+	sql_expr_list_delete(db, old_checks);
 
 	/* If not initializing, then create new Tarantool space.
 	 *
@@ -1979,6 +1995,15 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		}
 #endif
 	}
+
+	/*
+	 * 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.
+	 */
+	sql_expr_list_delete(db, p->def->opts.checks);
+	p->def->opts.checks = NULL;
 }
 
 #ifndef SQLITE_OMIT_VIEW
@@ -2020,7 +2045,7 @@ sqlite3CreateView(Parse * pParse,	/* The parsing context */
 	 */
 	p->pSelect = sqlite3SelectDup(db, pSelect, EXPRDUP_REDUCE);
 	p->def->opts.is_view = true;
-	p->pCheck = sql_expr_list_dup(db, pCNames, EXPRDUP_REDUCE);
+	p->def->opts.checks = sql_expr_list_dup(db, pCNames, EXPRDUP_REDUCE);
 	if (db->mallocFailed)
 		goto create_view_fail;
 
@@ -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);
+	if (checks != NULL) {
 		/* CREATE VIEW name(arglist) AS ...
 		 * The names of the columns in the table are taken
 		 * from arglist which is stored in table->pCheck.
@@ -2115,7 +2142,7 @@ sql_view_column_names(struct Parse *parse, struct Table *table)
 		 * VIEW it holds the list of column names.
 		 */
 		struct space_def *old_def = table->def;
-		sqlite3ColumnsFromExprList(parse, table->pCheck, table);
+		sqlite3ColumnsFromExprList(parse, checks, table);
 		if (sql_table_def_rebuild(db, table) != 0) {
 			rc = -1;
 		} else {
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 6e7144c..f78f679 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1164,21 +1164,23 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 		}
 	}
 
-	/* Test all CHECK constraints
+	/*
+	 * Get server checks.
+	 * Test all CHECK constraints.
 	 */
-#ifndef SQLITE_OMIT_CHECK
-	if (pTab->pCheck && (user_session->sql_flags &
-			     SQLITE_IgnoreChecks) == 0) {
-		ExprList *pCheck = pTab->pCheck;
+	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
+	ExprList *checks = space_checks_expr_list(space_id);
+	if (checks != NULL && (user_session->sql_flags &
+		       SQLITE_IgnoreChecks) == 0) {
 		pParse->ckBase = regNewData + 1;
 		if (override_error != ON_CONFLICT_ACTION_DEFAULT)
 			on_error = override_error;
 		else
 			on_error = ON_CONFLICT_ACTION_ABORT;
 
-		for (i = 0; i < pCheck->nExpr; i++) {
+		for (i = 0; i < checks->nExpr; i++) {
 			int allOk;
-			Expr *pExpr = pCheck->a[i].pExpr;
+			Expr *pExpr = checks->a[i].pExpr;
 			if (aiChng
 			    && checkConstraintUnchanged(pExpr, aiChng))
 				continue;
@@ -1188,7 +1190,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			if (on_error == ON_CONFLICT_ACTION_IGNORE) {
 				sqlite3VdbeGoto(v, ignoreDest);
 			} else {
-				char *zName = pCheck->a[i].zName;
+				char *zName = checks->a[i].zName;
 				if (zName == 0)
 					zName = pTab->def->name;
 				if (on_error == ON_CONFLICT_ACTION_REPLACE)
@@ -1202,7 +1204,6 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			sqlite3VdbeResolveLabel(v, allOk);
 		}
 	}
-#endif				/* !defined(SQLITE_OMIT_CHECK) */
 
 	/* Test all UNIQUE constraints by creating entries for each UNIQUE
 	 * index and making sure that duplicate entries do not already exist.
@@ -1886,12 +1887,16 @@ xferOptimization(Parse * pParse,	/* Parser context */
 			return 0;	/* pDestIdx has no corresponding index in pSrc */
 		}
 	}
-#ifndef SQLITE_OMIT_CHECK
-	if (pDest->pCheck
-	    && sqlite3ExprListCompare(pSrc->pCheck, pDest->pCheck, -1)) {
-		return 0;	/* Tables have different CHECK constraints.  Ticket #2252 */
+	/* Get server checks. */
+	ExprList *pCheck_src = space_checks_expr_list(
+		SQLITE_PAGENO_TO_SPACEID(pSrc->tnum));
+	ExprList *pCheck_dest = space_checks_expr_list(
+		SQLITE_PAGENO_TO_SPACEID(pDest->tnum));
+	if (pCheck_dest
+	    && sqlite3ExprListCompare(pCheck_src, pCheck_dest, -1)) {
+		/* Tables have different CHECK constraints.  Ticket #2252 */
+		return 0;
 	}
-#endif
 #ifndef SQLITE_OMIT_FOREIGN_KEY
 	/* Disallow the transfer optimization if the destination table constains
 	 * any foreign key constraints.  This is more restrictive than necessary.
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index eb7c93b..f966018 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -166,14 +166,12 @@ static const PragmaName aPragmaName[] = {
 	 /* iArg:      */ SQLITE_FullColNames},
 #endif
 #if !defined(SQLITE_OMIT_FLAG_PRAGMAS)
-#if !defined(SQLITE_OMIT_CHECK)
 	{ /* zName:     */ "ignore_check_constraints",
 	 /* ePragTyp:  */ PragTyp_FLAG,
 	 /* ePragFlg:  */ PragFlg_Result0 | PragFlg_NoColumns1,
 	 /* ColNames:  */ 0, 0,
 	 /* iArg:      */ SQLITE_IgnoreChecks},
 #endif
-#endif
 #if !defined(SQLITE_OMIT_SCHEMA_PRAGMAS)
 	{ /* zName:     */ "index_info",
 	 /* ePragTyp:  */ PragTyp_INDEX_INFO,
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index fd82119..0523b4e 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
 		else if (pNC->ncFlags & NC_IsCheck)
 			zIn = "CHECK constraints";
-#endif
 		sqlite3ErrorMsg(pParse, "%s prohibited in %s", zMsg, zIn);
 	}
 }
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 5746cb4..7611b4c 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1924,7 +1924,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 */
diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
index 9e21c55..82d0fb7 100755
--- a/test/sql-tap/check.test.lua
+++ b/test/sql-tap/check.test.lua
@@ -345,7 +345,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-3.1>
-        1, "subqueries prohibited in CHECK constraints"
+        1, "Failed to create space 'T3': subqueries prohibited in CHECK constraints"
         -- </check-3.1>
     })
 
@@ -370,7 +370,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-3.3>
-        1, "no such column: Q"
+        1, "Failed to create space 'T3': no such column: Q"
         -- </check-3.3>
     })
 
@@ -394,7 +394,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-3.5>
-        1, "no such column: T2.X"
+        1, "Failed to create space 'T3': no such column: T2.X"
         -- </check-3.5>
     })
 
@@ -555,7 +555,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-5.1>
-        1, "parameters prohibited in CHECK constraints"
+        1, "Failed to create space 'T5': parameters prohibited in CHECK constraints"
         -- </check-5.1>
     })
 
@@ -567,7 +567,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-5.2>
-        1, "parameters prohibited in CHECK constraints"
+        1, "Failed to create space 'T5': parameters prohibited in CHECK constraints"
         -- </check-5.2>
     })
 
-- 
2.7.4

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

[Vladislav Shpilevoy]

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.

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

[Kirill Shcherbatov]

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

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

[Vladislav Shpilevoy]

Thanks for the fixes! See my 3 comments below.

And I have pushed several review fix commits. Look at them
and squash.

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

1. I do not see a test on it.
> 
>> 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.

2. Are you sure? I added assert(def->checks == NULL) in deleteTable() and it fails:

box.sql.execute("CREATE TABLE t1(x INTEGER CHECK( x<5 ), y REAL CHECK( y>x ), z primary key, invalid:parameter);")

Assertion failed: (pTable->def->opts.checks == NULL), function deleteTable, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/build.c, line 414.
Process 24676 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
     frame #0: 0x00007fff5eed8b6e libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff5eed8b6e <+10>: jae    0x7fff5eed8b78            ; <+20>
     0x7fff5eed8b70 <+12>: movq   %rax, %rdi
     0x7fff5eed8b73 <+15>: jmp    0x7fff5eecfb00            ; cerror_nocancel
     0x7fff5eed8b78 <+20>: retq

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

3. One test is not enough. You did not test my example with 'exp' above, you did not test 'expr'
type mismatch, unexpected options in a check (expr + name + some unexpected one) etc.

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

[Kirill Shcherbatov]

> 1. I do not see a test on it
> 3. One test is not enough. You did not test my example with 'exp' above, you did not test 'expr'
> type mismatch, unexpected options in a check (expr + name + some unexpected one) etc.
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -18,4 +18,27 @@ t = {513, 1, 'test', 'memtx', 0, opts, format}
 s = box.space._space:insert(t)
 box.space._space:delete(513)
 
+opts = {checks = {{expr = 'X>5', name = 'ONE'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+box.space._space:delete(513)
+
+-- extra invlalid field name
+opts = {checks = {{expr = 'X>5', name = 'ONE', extra = 'TWO'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
+opts = {checks = {{expr_invalid_label = 'X>5'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
+-- invalid field type
+opts = {checks = {{name = 123}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)


>> 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.
> 2. Are you sure? I added assert(def->checks == NULL) in deleteTable() and it fails:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index bd99f20..8acd01e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -408,10 +408,11 @@ deleteTable(sqlite3 * db, Table * pTable)
        sqlite3SelectDelete(db, pTable->pSelect);
        assert(pTable->def != NULL);
        /* Do not delete pTable->def allocated on region. */
-       if (!pTable->def->opts.temporary)
+       if (!pTable->def->opts.temporary) {
                space_def_delete(pTable->def);
-       else
-               assert(pTable->def->opts.checks == NULL);
+       } else {
+               sql_expr_list_delete(db, pTable->def->opts.checks);
+       }
        sqlite3DbFree(db, pTable);

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

[Vladislav Shpilevoy]

Thanks for the fixes! I force pushed new ones on the branch. Now the
patchset LGTM.

Nikita, please, review it.

On 28/05/2018 17:59, Kirill Shcherbatov wrote:
>> 1. I do not see a test on it
>> 3. One test is not enough. You did not test my example with 'exp' above, you did not test 'expr'
>> type mismatch, unexpected options in a check (expr + name + some unexpected one) etc.
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -18,4 +18,27 @@ t = {513, 1, 'test', 'memtx', 0, opts, format}
>   s = box.space._space:insert(t)
>   box.space._space:delete(513)
>   
> +opts = {checks = {{expr = 'X>5', name = 'ONE'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +box.space._space:delete(513)
> +
> +-- extra invlalid field name
> +opts = {checks = {{expr = 'X>5', name = 'ONE', extra = 'TWO'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +
> +opts = {checks = {{expr_invalid_label = 'X>5'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +
> +-- invalid field type
> +opts = {checks = {{name = 123}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> 
> 
>>> 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.
>> 2. Are you sure? I added assert(def->checks == NULL) in deleteTable() and it fails:
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bd99f20..8acd01e 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -408,10 +408,11 @@ deleteTable(sqlite3 * db, Table * pTable)
>          sqlite3SelectDelete(db, pTable->pSelect);
>          assert(pTable->def != NULL);
>          /* Do not delete pTable->def allocated on region. */
> -       if (!pTable->def->opts.temporary)
> +       if (!pTable->def->opts.temporary) {
>                  space_def_delete(pTable->def);
> -       else
> -               assert(pTable->def->opts.checks == NULL);
> +       } else {
> +               sql_expr_list_delete(db, pTable->def->opts.checks);
> +       }
>          sqlite3DbFree(db, pTable);
> 

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

[n.pettik]

>sql: remove Checks to server

I guess, it would be better to use word ‘move’...

> 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"}, ..].
> + * @param len array items count.
> + * @param opt pointer to store parsing result.

You can specify that ‘opt’ is output param by [out] tag:
@param[out] opt ...

>+++ b/src/box/space_def.h
>@@ -40,6 +40,8 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
> 
>+struct ExprList;
>+

Mb it would be better to rename this struct according to our codestyle?

>@@ -79,12 +81,8 @@ space_opts_create(struct space_opts *opts)
> /**
>  * Destroy space options
>  */
>-static inline void
>-space_opts_destroy(struct space_opts *opts)
>-{
>-       free(opts->sql);
>-       TRASH(opts);
>-}
>+void
>+space_opts_destroy(struct space_opts *opts);

Seems like this function lacks some meaningful comment.

>+++ b/src/box/sql.c
>@@ -1500,22 +1500,47 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>  *
>  * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
>  */
>-int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
>+int
>+tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
> {
>        const struct Enc *enc = get_enc(buf);
>-       char *base = buf, *p;
>+       bool is_view = pTable != NULL && pTable->def->opts.is_view;
>+       bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;

In fact, it is incorrect since opts.checks may also contain aliases for VIEW
columns: CREATE VIEW V1(a, b, c) AS SELECT * FROM t1;
In this case, checks contain NULL exprs with names ‘A’, ‘B’ and ‘C’.
So you should add another one condition: && is_view.

>+               if (a[i].zName != NULL) {
>+                       p = enc->encode_str(p, "name", 4);
>+                       p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
>+               }

The same is here: you add checks for VIEW which are really aliases.

>+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;
>+
>+       sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+                                  expr_list);
>+
>+       sql_parser_destroy(&parser);
>+       if (parser.rc != SQLITE_OK) {
>+               /* Error is already set with diag. */
>+               if (parser.rc != SQL_TARANTOOL_ERROR)
>+                       diag_set(ClientError, ER_SQL, parser.zErrMsg);

Comment isn’t consistent with code: if error is already
set, why you set it again?

>@@ -232,6 +240,42 @@ void
> sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
>                           struct Expr *expr, struct ExprList *expr_list);
> 
>+/**
>+ * Initialize check_list_item.
>+ * @param expr_list ExprList with item.
>+ * @param idx item index.

I would somehow rename this var, since it is easy to
confuse one with things related to database indexes.

>+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;
>+
>+       sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+                                  expr_list);

Why do we still need to create table wrappers?
Is it possible to change signatures and use straight space_def instead of fake tables?

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

[Kirill Shcherbatov]

> You can specify that ‘opt’ is output param by [out] tag:
> @param[out] opt ...
- * @param opt pointer to store parsing result.
+ * @param[out] opt pointer to store parsing result.

> Mb it would be better to rename this struct according to our codestyle?
ExprList is a SQL structure not only for checks, so it's not a good idea to make a project-wide renaming. 
It is also not possible to rename it only in this place, because I don't like extra type casts thats are required in such scenario.

>> +void
>> +space_opts_destroy(struct space_opts *opts);
> 
> Seems like this function lacks some meaningful comment.
Here(in space_def.c) is just a function declaration since a definition is located in space_def.h and it match local code-style convention.  
> 
>> +++ b/src/box/sql.c
>> @@ -1500,22 +1500,47 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>>  *
>>  * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
>>  */
>> -int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
>> +int
>> +tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
>> {
>>        const struct Enc *enc = get_enc(buf);
>> -       char *base = buf, *p;
>> +       bool is_view = pTable != NULL && pTable->def->opts.is_view;
>> +       bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
> 
> In fact, it is incorrect since opts.checks may also contain aliases for VIEW
> columns: CREATE VIEW V1(a, b, c) AS SELECT * FROM t1;
> In this case, checks contain NULL exprs with names ‘A’, ‘B’ and ‘C’.
> So you should add another one condition: && is_view.

> The same is here: you add checks for VIEW which are really aliases.
The VIEW checks are also represented as ExprList with name pointers but lacks Expr pointer.
So, this check is valid. 
The small comment bellow is about it:
/*
 * a[i].pExpr could be NULL for VIEW column names
 * represented as checks.
*/


> Comment isn’t consistent with code: if error is already
> set, why you set it again?
-               /* Error is already set with diag. */
+               /* Tarantool error may be already set with diag. */


> I would somehow rename this var, since it is easy to
> confuse one with things related to database indexes.
+++ b/src/box/sql.h
@@ -243,7 +243,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
 /**
  * Initialize check_list_item.
  * @param expr_list ExprList with item.
- * @param idx item index.
+ * @param column index.
  * @param expr_name expression name (optional).
  * @param expr_name_len expresson name length (optional).
  * @param expr_str expression to build string.
@@ -252,7 +252,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
  * @retval -1 on error.
  */
 int
-sql_check_list_item_init(struct ExprList *expr_list, int idx,
+sql_check_list_item_init(struct ExprList *expr_list, int column,
                         const char *expr_name, uint32_t expr_name_len,
                         const char *expr_str, uint32_t expr_str_len);

+++ b/src/box/sql.c
@@ -1788,12 +1788,12 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
 }
 
 int
-sql_check_list_item_init(struct ExprList *expr_list, int idx,
+sql_check_list_item_init(struct ExprList *expr_list, int column,
                         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];
+       assert(column < expr_list->nExpr);
+       struct ExprList_item *item = &expr_list->a[column];

> Why do we still need to create table wrappers?
> Is it possible to change signatures and use straight space_def instead of fake tables?
sql_resolve_self_reference initialize SrcList field pTab; 
Unfortunately sql_resolve_self_reference is not the only way of usage  SrcList and at least Table field pIndex is required in other scenarios.
Being unable to remove pTable pointer I've fellowed this way.

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

[n.pettik]

> On 30 May 2018, at 11:32, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
>> You can specify that ‘opt’ is output param by [out] tag:
>> @param[out] opt ...
> - * @param opt pointer to store parsing result.
> + * @param[out] opt pointer to store parsing result.
> 
>> Mb it would be better to rename this struct according to our codestyle?
> ExprList is a SQL structure not only for checks, so it's not a good idea to make a project-wide renaming. 

But sooner or later we will have to do it. Anyway, as you wish.

>> 
>>> +++ b/src/box/sql.c
>>> @@ -1500,22 +1500,47 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>>> *
>>> * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
>>> */
>>> -int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
>>> +int
>>> +tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
>>> {
>>>       const struct Enc *enc = get_enc(buf);
>>> -       char *base = buf, *p;
>>> +       bool is_view = pTable != NULL && pTable->def->opts.is_view;
>>> +       bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
>> 
>> In fact, it is incorrect since opts.checks may also contain aliases for VIEW
>> columns: CREATE VIEW V1(a, b, c) AS SELECT * FROM t1;
>> In this case, checks contain NULL exprs with names ‘A’, ‘B’ and ‘C’.
>> So you should add another one condition: && is_view.
> 
>> The same is here: you add checks for VIEW which are really aliases.
> The VIEW checks are also represented as ExprList with name pointers but lacks Expr pointer.
> So, this check is valid. 

I extremely dislike this original SQLite approach, since it is so confusing.
I suggest to rework it somehow. I have dived into this problem.
Now aliases are stored as checks only for
one reason: to tell whether SELECT of view is resolved or not.
If not, aliases are extracted from checks list and put into regular fields
array. Thus, if we store aliases as fields right after view creation,
we can’t point out whether we have resolved names for this view,
or it is just view with aliases.

But since point of this patch slightly different, lets leave it as it is.

> 
>> Why do we still need to create table wrappers?
>> Is it possible to change signatures and use straight space_def instead of fake tables?
> sql_resolve_self_reference initialize SrcList field pTab; 
> Unfortunately sql_resolve_self_reference is not the only way of usage  SrcList and at least Table field pIndex is required in other scenarios.

Well, l see now. Seems it can be fixed after introducing index_def in struct Index.

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

[Kirill Shcherbatov]

From 63875ce19a0eababd765680ef46463b2ccf92905 Mon Sep 17 00:00:00 2001
Message-Id: <63875ce19a0eababd765680ef46463b2ccf92905.1527249763.git.kshcherbatov@tarantool.org>
In-Reply-To: <cover.1527084286.git.kshcherbatov@tarantool.org>
References: <cover.1527084286.git.kshcherbatov@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Fri, 25 May 2018 11:41:32 +0300
Subject: [PATCH 1/8] sql: fix memory leaks in hash and on OP_RenameTable

---
 src/box/sql/hash.c | 1 +
 src/box/sql/vdbe.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/box/sql/hash.c b/src/box/sql/hash.c
index b6f3ff3..84dfc36 100644
--- a/src/box/sql/hash.c
+++ b/src/box/sql/hash.c
@@ -238,6 +238,7 @@ removeElementGivenHash(Hash * pH,	/* The pH containing "elem" */
 		pEntry->count--;
 		assert(pEntry->count >= 0);
 	}
+	free(elem->pKey);
 	sqlite3_free(elem);
 	pH->count--;
 	if (pH->count == 0) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3a1e2e0..3fe5875 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4804,6 +4804,7 @@ case OP_RenameTable: {
 
 	/* Rename all trigger created on this table.*/
 	for (; pTrig; pTrig = pTrig->pNext) {
+		sqlite3DbFree(db, pTrig->table);
 		pTrig->table = sqlite3DbStrNDup(db, zNewTableName,
 						sqlite3Strlen30(zNewTableName));
 		pTrig->pTabSchema = pTab->pSchema;
-- 
2.7.4

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

[Vladislav Shpilevoy]

Thanks for the patch!

On 25/05/2018 15:04, Kirill Shcherbatov wrote:
>  From 63875ce19a0eababd765680ef46463b2ccf92905 Mon Sep 17 00:00:00 2001
> Message-Id: <63875ce19a0eababd765680ef46463b2ccf92905.1527249763.git.kshcherbatov@tarantool.org>
> In-Reply-To: <cover.1527084286.git.kshcherbatov@tarantool.org>
> References: <cover.1527084286.git.kshcherbatov@tarantool.org>
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Fri, 25 May 2018 11:41:32 +0300
> Subject: [PATCH 1/8] sql: fix memory leaks in hash and on OP_RenameTable

1. Why do I see SMTP headers in the message body?

The patch is LGTM.

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

[n.pettik]

Now the whole patch-set LGTM.

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

[Kirill Yukhin]

Hello,
On 30 мая 14:03, n.pettik wrote:
> Now the whole patch-set LGTM.
I've checked the patch-set into 2.0.

--
Regards, Kirill Yukhin