From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 788BC29679 for ; Thu, 29 Mar 2018 10:00:38 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gkyf0fqI80Hy for ; Thu, 29 Mar 2018 10:00:38 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1111D29671 for ; Thu, 29 Mar 2018 10:00:37 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: move default col values to Tarantool's core From: "v.shpilevoy@tarantool.org" In-Reply-To: <83c7cbe4d5757ef1efa49cae732d0969b4f69c94.1522303843.git.kyukhin@tarantool.org> Date: Thu, 29 Mar 2018 17:00:29 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <83c7cbe4d5757ef1efa49cae732d0969b4f69c94.1522303843.git.kyukhin@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Kirill Yukhin See below 20 comments. > 29 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2018 =D0=B3., =D0=B2 9:42, Kirill = Yukhin =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(= =D0=B0): >=20 > Extract expressions parsing into separate routine to > allow Tarantool's backend compile DEFAULT statements w/o > SQL machinery at all. So far, for DEFAULT values no context > is needed: only constant expressions and built-ins are allowed. > In future, table context will be added to allow use column > values for CHECK constraint expressions. >=20 > Move DEFAULT string value to space_def. Move compiled expresion > to field_def as well. Reason not to move compiled expression > to tuple_field as follows: we do not want to engage parser > during tuple validation. So, do it in alter.cc. >=20 > In order to allow expression duplication in alter.cc: expose > those routines from expr.c and make their names correspond to > coding style. >=20 > Since recovery is performed before SQL fronend initialization: > split it into two pieces: 1. create SQL handler, enable > all subsystems 2. Do recovery. This will allow to run > parser during recovery, since it needs db handle so far. >=20 > Part of #3235 > --- > src/CMakeLists.txt | 2 +- > src/box/alter.cc | 17 + > src/box/box.cc | 3 +- > src/box/field_def.c | 5 +- > src/box/field_def.h | 4 + > src/box/space_def.c | 105 ++++-- > src/box/space_def.h | 10 +- > src/box/sql.c | 59 +++- > src/box/sql.h | 30 ++ > src/box/sql/build.c | 18 +- > src/box/sql/delete.c | 12 +- > src/box/sql/expr.c | 114 ++++--- > src/box/sql/fkey.c | 13 +- > src/box/sql/insert.c | 32 +- > src/box/sql/main.c | 15 - > src/box/sql/parse.c | 876 = ++++++++++++++++++++++++------------------------ > src/box/sql/parse.y | 32 +- > src/box/sql/resolve.c | 12 +- > src/box/sql/select.c | 28 +- > src/box/sql/sqliteInt.h | 11 +- > src/box/sql/tokenize.c | 121 +++++++ > src/box/sql/trigger.c | 12 +- > src/box/sql/update.c | 2 +- > src/box/sql/wherecode.c | 2 +- > src/box/sql/whereexpr.c | 6 +- > 25 files changed, 928 insertions(+), 613 deletions(-) >=20 > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 8ab09e9..eae40c3 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > @@ -268,7 +268,7 @@ add_executable( > ${LIBUTIL_FREEBSD_SRC}/flopen.c > ${LIBUTIL_FREEBSD_SRC}/pidfile.c) >=20 > -add_dependencies(tarantool build_bundled_libs preprocess_exports) > +add_dependencies(tarantool build_bundled_libs preprocess_exports sql) >=20 > # Re-link if exports changed > set_target_properties(tarantool PROPERTIES LINK_DEPENDS = ${exports_file}) > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 299dcb0..b976d0f 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -52,6 +52,7 @@ > #include "memtx_tuple.h" > #include "version.h" > #include "sequence.h" > +#include "sql.h" >=20 > /** > * chap-sha1 of empty string, i.e. > @@ -403,6 +404,22 @@ field_def_decode(struct field_def *field, const = char **data, > tt_sprintf("collation is reasonable only for " > "string, scalar and any fields")); > } > + > + if (field->default_value !=3D NULL) { > + struct Expr *expr =3D NULL; > + char *err =3D 0; 1. err =3D NUlL. > + /* Recovery is performed before SQL FE is initialized. > + * Postpone AST compilation if in process of recovery. > + */ > + if (sql_get() !=3D NULL) { 1. In the commit message you said:=20 "Since recovery is performed before SQL fronend initialization: split it into two pieces: 1. create SQL handler, enable all subsystems 2. Do recovery. This will allow to run parser during recovery, since it needs db handle so far." Why sql_get() can be NULL here? And why do you ignore error? In a case of error in one of next field definitions, or because of OOM, or error further in space_def_new_from_tuple, the default_value_ast leaks. Other field_def fields are not deleted explicitly here, because = they are on region memory, that is deleted on a transaction rollback. > + sql_compile_expr(sql_get(), > + field->default_value, > + &expr, > + &err); > + assert(err =3D=3D NULL); > + field->default_value_ast =3D expr; 2. How about another name? For example, default_value_expr? I know, formally it is AST, but actually it is expr. Or lets rename struct Expr = to struct expr_ast, or struct sql_ast or something. > + } > + } > } >=20 > /** >=20 > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 1b3c305..7b9a664 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -32,6 +32,7 @@ > #include "space_def.h" > #include "diag.h" > #include "error.h" > +#include "sql.h" >=20 > const struct space_opts space_opts_default =3D { > /* .temporary =3D */ false, > @@ -49,7 +50,7 @@ const struct opt_def space_opts_reg[] =3D { > /** > * Size of the space_def. > * @param name_len Length of the space name. > - * @param field_names_size Size of all names. > + * @param fields Fields array of space format. > * @param field_count Space field count. > * @param[out] names_offset Offset from the beginning of a def to > * a field names memory. 3. Add a comment about second out parameter, please. > @@ -58,25 +59,37 @@ const struct opt_def space_opts_reg[] =3D { > * @retval Size in bytes. > */ > static inline size_t > -space_def_sizeof(uint32_t name_len, uint32_t field_names_size, > +space_def_sizeof(uint32_t name_len, const struct field_def *fields, > uint32_t field_count, uint32_t *names_offset, > - uint32_t *fields_offset) > + uint32_t *fields_offset, uint32_t *def_ast_offset) > { > + uint32_t field_strs_size =3D 0; > + uint32_t def_exprs_size =3D 0; > + for (uint32_t i =3D 0; i < field_count; ++i) { > + field_strs_size +=3D strlen(fields[i].name) + 1; > + if (fields[i].default_value !=3D NULL) { > + int len =3D strlen(fields[i].default_value); > + field_strs_size +=3D len + 1; > + } > + if (fields[i].default_value_ast !=3D NULL) { 4. If I correctly understand, (fields[i].default_value !=3D NULL) =3D=3D (fields[i].default_value_ast !=3D NULL). So if a default_value !=3D NULL, then it is guaranteed the default_value_ast !=3D NULL too. Please, correct me, or move this code into the body of the previous 'if' and add an assertion. > + struct Expr *expr =3D = fields[i].default_value_ast; > + def_exprs_size +=3D sql_duped_expr_size(expr, = 0); > + } > + } > + > *fields_offset =3D sizeof(struct space_def) + name_len + 1; > *names_offset =3D *fields_offset + field_count * sizeof(struct = field_def); > - return *names_offset + field_names_size; > + *def_ast_offset =3D *names_offset + field_strs_size; > + return *def_ast_offset + def_exprs_size; > } >=20 > struct space_def * > space_def_dup(const struct space_def *src) > { > - uint32_t names_offset, fields_offset; > - uint32_t field_names_size =3D 0; > - for (uint32_t i =3D 0; i < src->field_count; ++i) > - field_names_size +=3D strlen(src->fields[i].name) + 1; > - size_t size =3D space_def_sizeof(strlen(src->name), = field_names_size, > - src->field_count, &names_offset, > - &fields_offset); > + uint32_t strs_offset, fields_offset, def_ast_offset; > + size_t size =3D space_def_sizeof(strlen(src->name), src->fields, > + src->field_count, &strs_offset, > + &fields_offset, &def_ast_offset); > struct space_def *ret =3D (struct space_def *) malloc(size); > if (ret =3D=3D NULL) { > diag_set(OutOfMemory, size, "malloc", "ret"); > @@ -92,12 +105,25 @@ space_def_dup(const struct space_def *src) > return NULL; > } > } > - char *name_pos =3D (char *)ret + names_offset; > + char *strs_pos =3D (char *)ret + strs_offset; > + char *expr_pos =3D (char *)ret + def_ast_offset; > if (src->field_count > 0) { > ret->fields =3D (struct field_def *)((char *)ret + = fields_offset); > for (uint32_t i =3D 0; i < src->field_count; ++i) { > - ret->fields[i].name =3D name_pos; > - name_pos +=3D strlen(name_pos) + 1; > + ret->fields[i].name =3D strs_pos; > + strs_pos +=3D strlen(strs_pos) + 1; > + if (src->fields[i].default_value !=3D NULL) { > + ret->fields[i].default_value =3D = strs_pos; > + strs_pos +=3D strlen(strs_pos) + 1; > + } 5. Same as 3. > + struct Expr *def_expr_ast =3D = src->fields[i].default_value_ast; > + if (def_expr_ast !=3D NULL) { > + struct Expr *expr =3D NULL; 6. Useless initialization. > + expr =3D sql_expr_dup(sql_get(), = def_expr_ast, > + 0, &expr_pos); 7. If I correctly understand the sql_expr_dup, after the expression is = duplicated, expr_pos points the byte right after the duplicated Expr. Can you please = check it? If it is true, then you must skip this line "expr_pos +=3D = sql_duped_expr_size(expr, 0);", and I can not understand why it works now. > + ret->fields[i].default_value_ast =3D = expr; > + expr_pos +=3D sql_duped_expr_size(expr, = 0); > + } > } > } > tuple_dictionary_ref(ret->dict); > @@ -111,12 +137,10 @@ space_def_new(uint32_t id, uint32_t uid, = uint32_t exact_field_count, > const struct space_opts *opts, const struct field_def = *fields, > uint32_t field_count) > { > - uint32_t field_names_size =3D 0; > - for (uint32_t i =3D 0; i < field_count; ++i) > - field_names_size +=3D strlen(fields[i].name) + 1; > - uint32_t names_offset, fields_offset; > - size_t size =3D space_def_sizeof(name_len, field_names_size, = field_count, > - &names_offset, &fields_offset); > + uint32_t strs_offset, fields_offset, def_ast_offset; > + size_t size =3D space_def_sizeof(name_len, fields, field_count, > + &strs_offset, &fields_offset, > + &def_ast_offset); > struct space_def *def =3D (struct space_def *) malloc(size); > if (def =3D=3D NULL) { > diag_set(OutOfMemory, size, "malloc", "def"); > @@ -147,18 +171,35 @@ space_def_new(uint32_t id, uint32_t uid, = uint32_t exact_field_count, > } > } > def->field_count =3D field_count; > - if (field_count =3D=3D 0) { > + if (field_count =3D=3D 0) > def->fields =3D NULL; > - } else { > - char *name_pos =3D (char *)def + names_offset; > + else { > + char *strs_pos =3D (char *)def + strs_offset; > + char *expr_pos =3D (char *)def + def_ast_offset; > def->fields =3D (struct field_def *)((char *)def + = fields_offset); > for (uint32_t i =3D 0; i < field_count; ++i) { > def->fields[i] =3D fields[i]; > - def->fields[i].name =3D name_pos; > + def->fields[i].name =3D strs_pos; > uint32_t len =3D strlen(fields[i].name); > memcpy(def->fields[i].name, fields[i].name, = len); > def->fields[i].name[len] =3D 0; > - name_pos +=3D len + 1; > + strs_pos +=3D len + 1; > + > + if (fields[i].default_value !=3D NULL) { > + def->fields[i].default_value =3D = strs_pos; > + len =3D strlen(fields[i].default_value); > + memcpy(def->fields[i].default_value, = fields[i].default_value, len); > + def->fields[i].default_value[len] =3D 0; > + strs_pos +=3D len + 1; > + } 8. Same as 5, 6, 7. > + struct Expr *def_expr_ast =3D = fields[i].default_value_ast; > + if (def_expr_ast !=3D NULL) { > + struct Expr *expr =3D NULL; > + expr =3D sql_expr_dup(sql_get(), = def_expr_ast, > + 0, &expr_pos); > + def->fields[i].default_value_ast =3D = expr; > + expr_pos +=3D sql_duped_expr_size(expr, = 0); > + } > } > } > return def; > @@ -217,3 +258,17 @@ space_def_check_compatibility(const struct = space_def *old_def, > return 0; > } >=20 > +void > +space_def_delete(struct space_def *def) > +{ > + space_opts_destroy(&def->opts); > + tuple_dictionary_unref(def->dict); > + for (uint32_t i =3D 0; i < def->field_count; ++i) { > + if (def->fields[i].default_value_ast !=3D NULL) { > + sql_expr_free(sql_get(), > + def->fields[i].default_value_ast); > + } 9. Why does it work, if your expressions are the part of megamallocs = above? > + } > + TRASH(def); > + free(def); > +} > diff --git a/src/box/sql.c b/src/box/sql.c > index 6d8ef9a..8606be6 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -56,7 +56,7 @@ > #include "xrow.h" > #include "iproto_constants.h" >=20 > -static sqlite3 *db; > +static sqlite3 *db =3D 0; 10. Please, use NULL instead of 0. Common comment: assigning 0 in the code, where the variable declaration is not strictly visible is very confusing - it seems to be integer. > @@ -1781,3 +1814,25 @@ tarantoolSqlGetMaxId(uint32_t space_id, = uint32_t index_id, uint32_t fieldno, >=20 > return tuple_field_u64(tuple, fieldno, max_id); > } > + > +/** > + * Given space_id and field number, return default value > + * for the field. > + * @param space_id Space ID. > + * @param fieldno Field index. > + * @retval Pointer to AST corresponding to default value. > + * Can be NULL if no DEFAULT specified or it is a view. > + */ 11. Please, put a function description above a declaration instead of = implementation. We put description above implementation only if a declaration is absent. > +struct Expr* > +space_get_column_default_expr(uint32_t space_id, uint32_t fieldno) > +{ > + struct space *space; > + space =3D space_cache_find(space_id); > + assert(space !=3D NULL); > + assert(space->def !=3D NULL); > + if (space->def->opts.is_view) > + return NULL; 12. How is it possible? Must not it be an assertion? > + assert(space->def->field_count > fieldno); > + > + return space->def->fields[fieldno].default_value_ast; > +} > diff --git a/src/box/sql.h b/src/box/sql.h > @@ -55,6 +60,31 @@ sql_free(); > struct sqlite3 * > sql_get(); >=20 > +struct Expr; > +struct Parse; > +struct Select; > + > +int > +sql_compile_expr(struct sqlite3 *db, > + const char *expr, > + struct Expr **result, > + char **err); > + > +void > +sql_extract_select_expr(struct Parse *parser, struct Select *select); > + > +struct Expr* > +space_get_column_default_expr(uint32_t space_id, uint32_t fieldno); > + > +int > +sql_duped_expr_size(struct Expr *p, int flags); 13. How about simply expr_size, not duped? And please, name methods of a = specific struct using convention _. And for struct size methods use sizeof name (see space_def_sizeof, key_def_sizeof). For = 'free' methods we use name 'delete'. Delete means, that the struct's memory is freed. = Also we have 'destroy' methods, which frees only struct members, but not the = struct itself. Here it is expr_sizeof(), expr_delete(), expr_dup(). And please, do not use 'get' for getters. Here it is = space_column_default_expr(). > + > +struct Expr * > +sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char = **buffer); > + > +void > +sql_expr_free(struct sqlite3 *db, struct Expr *expr); > + > #if defined(__cplusplus) > } /* extern "C" { */ > #endif > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 05a7cc9..c3bd637 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1018,9 +1018,7 @@ sqlite3AddCheckConstraint(Parse * pParse, = /* Parsing context */ > } > } else > #endif > - { > - sqlite3ExprDelete(pParse->db, pCheckExpr); > - } > + sql_expr_free(pParse->db, pCheckExpr); 14. Extra tab. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 89dcb23..093f99e 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -492,7 +492,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. > * > - * sqlite3ExprDelete() specifically skips the recursive = delete of > + * sql_expr_free() 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 > @@ -935,8 +935,8 @@ sqlite3ExprAttachSubtrees(sqlite3 * db, > { > if (pRoot =3D=3D 0) { > assert(db->mallocFailed); > - sqlite3ExprDelete(db, pLeft); > - sqlite3ExprDelete(db, pRight); > + sql_expr_free(db, pLeft); > + sql_expr_free(db, pRight); > } else { > if (pRight) { > pRoot->pRight =3D pRight; > @@ -1052,8 +1052,8 @@ sqlite3ExprAnd(sqlite3 * db, Expr * pLeft, Expr = * pRight) > } else if (pRight =3D=3D 0) { > return pLeft; > } else if (exprAlwaysFalse(pLeft) || exprAlwaysFalse(pRight)) { > - sqlite3ExprDelete(db, pLeft); > - sqlite3ExprDelete(db, pRight); > + sql_expr_free(db, pLeft); > + sql_expr_free(db, pRight); > return sqlite3ExprAlloc(db, TK_INTEGER, = &sqlite3IntTokens[0], > 0); > } else { > @@ -1189,7 +1189,7 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p) > assert(p->x.pList =3D=3D 0 || p->pRight =3D=3D 0); > if (p->pLeft && p->op !=3D TK_SELECT_COLUMN) > sqlite3ExprDeleteNN(db, p->pLeft); > - sqlite3ExprDelete(db, p->pRight); > + sql_expr_free(db, p->pRight); > if (ExprHasProperty(p, EP_xIsSelect)) { > sqlite3SelectDelete(db, p->x.pSelect); > } else { > @@ -1203,11 +1203,16 @@ sqlite3ExprDeleteNN(sqlite3 * db, Expr * p) > } > } >=20 > +/** > + * Recursively free storage occupied by AST expr. > + * @param db SQL handle. > + * @param p Pointer to root node. > + */ 15. Same as 11. > void > -sqlite3ExprDelete(sqlite3 * db, Expr * p) > +sql_expr_free(sqlite3 *db, Expr *expr) > { > - if (p) > - sqlite3ExprDeleteNN(db, p); > + if (expr !=3D NULL) > + sqlite3ExprDeleteNN(db, expr); > } >=20 > /* >=20 > @@ -1401,29 +1412,28 @@ exprDup(sqlite3 * db, Expr * p, int dupFlags, = u8 ** pzBuffer) > if (ExprHasProperty(p, EP_xIsSelect)) { > pNew->x.pSelect =3D > sqlite3SelectDup(db, p->x.pSelect, > - dupFlags); > + flags); > } else { > pNew->x.pList =3D > sqlite3ExprListDup(db, p->x.pList, > - dupFlags); > + flags); > } > } >=20 > /* Fill in pNew->pLeft and pNew->pRight. */ > if (ExprHasProperty(pNew, EP_Reduced | EP_TokenOnly)) { > - zAlloc +=3D dupedExprNodeSize(p, dupFlags); > + zAlloc +=3D dupedExprNodeSize(p, flags); > if (!ExprHasProperty(pNew, EP_TokenOnly | = EP_Leaf)) { > pNew->pLeft =3D p->pLeft ? > - exprDup(db, p->pLeft, = EXPRDUP_REDUCE, > + sql_expr_dup(db, p->pLeft, = EXPRDUP_REDUCE, > &zAlloc) : 0; 16. Bad alignment. >=20 > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 42254dd..00e263a 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -674,10 +678,15 @@ sqlite3Insert(Parse * pParse, /* Parser = context */ > if (i =3D=3D pTab->iAutoIncPKey) > sqlite3VdbeAddOp2(v, OP_Integer, = -1, > regCols + i + = 1); > - else > + else { 17. If 'else' body is inside {}, then its 'if' body must be too, even if consists of single line. >=20 > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index e9c0b31..6d4ae7c 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -63,10 +63,12 @@ > * asterisks and the comment text. > */ >=20 > -#include > #include > -#include > + > +#include "box/field_def.h" > +#include "box/sql.h" > #include "box/txn.h" > +#include "trivia/util.h" >=20 > /* > * These #defines should enable >2GB file support on POSIX if the > @@ -3007,6 +3009,10 @@ struct Parse { > With *pWithToFree; /* Free this WITH object at the end of = the parse */ >=20 > bool initiateTTrans; /* Initiate Tarantool transaction */ > + /* If set - do not emit byte code at all, just parse. */ > + bool parse_only; > + /* If parse_only is set to true, store parsed expression. */ > + struct Expr *parsed_expr; 18. For struct members please use /** comments instead of /*. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index f810db4..ae52731 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -652,3 +652,124 @@ sqlite3RunParser(Parse * pParse, const char = *zSql, char **pzErrMsg) > assert(nErr =3D=3D 0 || pParse->rc !=3D SQLITE_OK); > return nErr; > } > + > +/** > + * Perform parsing of provided expression. This is done by > + * surrounding the expression w/ 'SELECT ' prefix and perform > + * convetional parsing. Then extract result expression value from > + * stuct Select and return it. > + * @param db SQL context handle. > + * @param expr Expression to parse. > + * @param[out] result Result: AST structure. > + * @param[out] err Error string if any. > + * > + * @retval Error code if any. > + */ > +int > +sql_compile_expr(sqlite3 *db, > + const char *expr, > + struct Expr **result, > + char **err) > +{ > + struct Parse parser; > + int nErr =3D 0; /* Number of errors encountered */ > + int i; /* Loop counter */ > + void *engine; /* The LEMON-generated LALR(1) parser */ > + int token_type; /* type of the next token */ > + int last_token_type =3D -1; /* type of the previous token */ > + int mxSqlLen; /* Max length of an SQL string */ > + > + assert(expr !=3D 0); > + mxSqlLen =3D db->aLimit[SQLITE_LIMIT_SQL_LENGTH]; > + memset(&parser, 0, sizeof(struct Parse)); > + parser.rc =3D SQLITE_OK; > + parser.zTail =3D expr; > + parser.db =3D db; > + parser.pToplevel =3D NULL; > + parser.parse_only =3D true; > + i =3D 0; > + assert(err !=3D 0); > + engine =3D sqlite3ParserAlloc(sqlite3Malloc); > + if (engine =3D=3D NULL) { > + sqlite3OomFault(db); > + return SQLITE_NOMEM_BKPT; > + } > + > + const char *outer =3D "SELECT "; > + char *stmt =3D malloc(strlen(outer) + strlen(expr) + 1); 19. Please, use region. You do this in a DDL transaction only, so region usage allows you to do not free this manually. > + if (stmt =3D=3D NULL) { > + sqlite3OomFault(db); > + return SQLITE_NOMEM_BKPT; > + } > + sprintf(stmt, "%s%s", outer, expr); > + while (1) { 20. While (true). For boolean variables and constants please use boolean values.