[tarantool-patches] Re: [PATCH v2 6/7] sql: refactor sql_expr_create to set diag

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

Thanks for the fixes!

Firstly, please, append that to the commit message:
"
After that patch there are basically 2 ways of errors
processing and forwarding:

     - Use diag only. It works for all the places out of
       src/box/sql, and for some functions inside it. For
       example, sql_expr_new();

     - Use global flags Parse.is_aborted, sql.mallocFailed.

It is easy to see, that some places use both of them
implicitly. For example, sql_expr_new() and every other
place which uses SQLite memory allocators + diag. But it
is ok until the former is removed. What is more important,
is that at least one of these two methods should be
consistent and finished in every functions. And match a
declared behaviour.

For example, it is incorrect to declare a function as setting
flags on an error, but in fact set diag only. Or vice versa,
that it throws a diag, but factually sets flags only.
"

Also see my comments below, fixes at the end of the email
and on the branch. While previous commits I've fixed silently,
there I have something to say.

> Refactored sqlExpr routine as sql_expr_new and reworked it to set
> diag message in case of memory allocation error. Also performed some
> additional name refactoring in adjacent places.
> This change is necessary because the sqlExpr body has a
> sqlNormalizeName call that will be changed in subsequent patches.
> 
> Part of #3931
> ---
>   src/box/sql/build.c         |  42 ++++--
>   src/box/sql/expr.c          | 248 ++++++++++++++----------------------
>   src/box/sql/fk_constraint.c | 190 +++++++++++++++++----------
>   src/box/sql/parse.y         |  51 ++++++--
>   src/box/sql/resolve.c       |  46 ++++---
>   src/box/sql/select.c        |  86 +++++++++----
>   src/box/sql/sqlInt.h        |  84 +++++++++++-
>   src/box/sql/wherecode.c     |   8 +-
>   src/box/sql/whereexpr.c     |  21 +--
>   9 files changed, 471 insertions(+), 305 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index f52cfd7dd..f527928bf 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c> @@ -1383,13 +1388,19 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>   	struct Expr *where = NULL;
>   	if (idx_name != NULL) {
>   		struct Expr *expr = sql_id_eq_str_expr(parse, "idx", idx_name);
> -		if (expr != NULL)
> -			where = sqlExprAnd(db, expr, where);
> +		if (expr != NULL && (expr != NULL || where != NULL)) {

1. This is what I meant and asked you to do - *self review*. Accurate,
attentive, diligent labor. But look at that condition:

     - You check 'expr' twice. If 'expr' != NULL first time, then the second
       time nothing has changed. What is more, the whole second part can be
       removed then;

     - 3 lines above 'where' is declared as NULL. And here you check if it
       is NULL.

It is not hard to find such mistakes. It requires just one single self
review, which usually removes most of such things.

Exactly the same about the code below.

Also, a couple of words about that pattern, that you
obviously just copy-pasted throughout the patch and got
pearls like above:

     if (a != NULL || b != NULL) {
         c = sql_and_expr_new(a, b);
         if (c != NULL)
             is_aborted = true
     }

It is bulky, hard to read and can be avoided. It appeared
because you forbid to pass NULL NULL into sql_and_expr_new.
But why? Because in such a case it returns NULL, but there
is no an error? Usually it means exactly an error, and only a
couple of places require such a check. Which can be done out of
sql_and_expr_new by the way.

> +			where = sql_and_expr_new(db, expr, where);
> +			if (where == NULL)
> +				parse->is_aborted = true;
> +		}
>   	}
>   	if (table_name != NULL) {
>   		struct Expr *expr = sql_id_eq_str_expr(parse, "tbl", table_name);
> -		if (expr != NULL)
> -			where = sqlExprAnd(db, expr, where);
> +		if (expr != NULL && (expr != NULL || where != NULL)) {
> +			where = sql_and_expr_new(db, expr, where);
> +			if (where == NULL)
> +				parse->is_aborted = true;

2. Above you have an assertion that either table_name != NULL, or
idx_name != NULL, or both. It means, that in a normal case 'where'
!= NULL at the end of the function. And if it is NULL, then
is_aborted can be set. Out of these 'if's. No code duplication.

> +		}
>   	}
>   	/**
>   	 * On memory allocation error sql_table delete_from
> @@ -2245,9 +2256,12 @@ sql_create_index(struct Parse *parse, struct Token *token,
>   		struct Token prev_col;
>   		uint32_t last_field = def->field_count - 1;
>   		sqlTokenInit(&prev_col, def->fields[last_field].name);
> -		col_list = sql_expr_list_append(parse->db, NULL,
> -						sqlExprAlloc(db, TK_ID,
> -								 &prev_col, 0));
> +		struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col, false);
> +		if (expr == NULL) {
> +			parse->is_aborted = true;
> +			goto exit_create_index;
> +		}
> +		col_list = sql_expr_list_append(parse->db, NULL, expr);

3. You used 'db' 5 lines above. I see, that it was in the old
code, but it is not an excuse. Now it is your code, and it should
be proper.

>   		if (col_list == NULL)
>   			goto exit_create_index;
>   		assert(col_list->nExpr == 1);
> diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
> index cc4dc6448..3b27b2d4f 100644
> --- a/src/box/sql/fk_constraint.c
> +++ b/src/box/sql/fk_constraint.c
> @@ -303,37 +303,36 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
>   
>   /**
>    * Return an Expr object that refers to a memory register
> - * corresponding to column iCol of given space.
> + * corresponding to column of given space.
>    *
> - * regBase is the first of an array of register that contains
> - * the data for given space.  regBase+1 holds the first column.
> - * regBase+2 holds the second column, and so forth.
> + * reg_base is the first of an array of register that contains
> + * the data for given space. reg_base+1 holds the first column.
> + * reg_base+2 holds the second column, and so forth.
>    *
> - * @param pParse Parsing and code generating context.
> + * @param parser The parsing context.
>    * @param def Definition of space whose content is at r[regBase]...
> - * @param regBase Contents of table defined by def.
> - * @param iCol Which column of space is desired.
> - * @return an Expr object that refers to a memory register
> - *         corresponding to column iCol of given space.
> + * @param reg_base Contents of table table.
> + * @param column Index of table column is desired.
> + * @retval Not NULL expression pointer on success.
> + * @retval NULL Otherwise.

4. Sorry. But sometimes I think, that probably you are
kidding. In the previous commits I after all fixed that
myself in scope of 'Review fixes'. But since here I do
a detailed review, not just a silent fix, I will say
that again:

     - Start sentences from a capital letter;

     - Do not profane comments writing. You write them
       just to write anything, but it should not be so.
       I asked you to rewrite places like this:

       /**
        * @retval Not NULL my_struct pointer on success.
        */
        struct my_struct *
        func(...)
        
       Because the information you have provided in that
       comment is useless. Even without the comment I see,
       that this function returns a my_struct pointer. The
       same about sql_register_expr_new - I see, that it
       returns struct Expr pointer. C language guarantees
       that.

       And it is not only about that place. Here it is
       simple. But sometimes a function can return not NULL
       on both error and success. And then you can not say
       'it returns a struct Expr pointer'. Or vice versa -
       return NULL on both success and error, like
       sql_and_expr_new.

     - Explicitly say if a function sets diagnostics area
       message. In SQL we are in a middle of two phases: when
       all the functions set diag, and when no one. This
       concrete function sets diag on any error.

>    */
> -static Expr *
> -space_field_register(Parse *pParse, struct space_def *def, int regBase,
> -		     i16 iCol)
> +static struct Expr *
> +sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base,
> +		      int column)>   {
> -	Expr *pExpr;
> -	sql *db = pParse->db;
> -
> -	pExpr = sqlExpr(db, TK_REGISTER, 0);
> -	if (pExpr) {
> -		if (iCol >= 0) {
> -			pExpr->iTable = regBase + iCol + 1;
> -			pExpr->type = def->fields[iCol].type;
> -		} else {
> -			pExpr->iTable = regBase;
> -			pExpr->type = FIELD_TYPE_INTEGER;
> -		}
> +	struct Expr *expr = sql_op_expr_new(parser->db, TK_REGISTER, NULL);
> +	if (expr == NULL) {
> +		parser->is_aborted = true;
> +		return NULL;
> +	}
> +	if (column >= 0) {

5. When I first saw that check I wondered how is it possible that
a column number is negative? Appeared that there is no way for that.
I've added an assertion about column >= 0 and the tests passed. What
is more, in all places where that function is called, it takes
uint32_t. So this check and the 'else' body are not necessary.

> +		expr->iTable = reg_base + column + 1;
> +		expr->type = def->fields[column].type;
> +	} else {
> +		expr->iTable = reg_base;
> +		expr->type = FIELD_TYPE_INTEGER;
>   	}
> -	return pExpr;
> +	return expr;
>   }
>   
>   /**
> @@ -435,12 +435,18 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src,
>   	for (uint32_t i = 0; i < fk_def->field_count; i++) {
>   		uint32_t fieldno = fk_def->links[i].parent_field;
>   		struct Expr *pexpr =
> -			space_field_register(parser, def, reg_data, fieldno);
> +			sql_register_expr_new(parser, def, reg_data, fieldno);
>   		fieldno = fk_def->links[i].child_field;
>   		const char *field_name = child_space->def->fields[fieldno].name;
> -		struct Expr *chexpr = sqlExpr(db, TK_ID, field_name);
> +		struct Expr *chexpr = sql_op_expr_new(db, TK_ID, field_name);
> +		if (chexpr == NULL)
> +			parser->is_aborted = true;
>   		struct Expr *eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr);
> -		where = sqlExprAnd(db, where, eq);
> +		if (where != NULL || eq != NULL) {

6. You do not need that check here, because if eq == NULL,
then pexpr and cheexpr are nulls. If they are nulls, then it
was an error, and it is ok to set is_aborted. Even if it is
already set. The same in many other places. Of course, after
revert of sql_and_expr_new asserting on NULL NULL.

> +			where = sql_and_expr_new(db, where, eq);
> +			if (where == NULL)
> +				parser->is_aborted = true;
> +		}
>   	}
>   
>   	/*> @@ -785,14 +802,26 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
>   		 * type and collation sequence associated with
>   		 * the parent table are used for the comparison.
>   		 */
> -		struct Expr *to_col =
> -			sqlPExpr(pParse, TK_DOT,
> -				     sqlExprAlloc(db, TK_ID, &t_old, 0),
> -				     sqlExprAlloc(db, TK_ID, &t_to_col, 0));
> -		struct Expr *from_col =
> -			sqlExprAlloc(db, TK_ID, &t_from_col, 0);
> -		struct Expr *eq = sqlPExpr(pParse, TK_EQ, to_col, from_col);
> -		where = sqlExprAnd(db, where, eq);
> +		struct Expr *old_expr = NULL, *new_expr = NULL, *expr = NULL;
> +		old_expr = sql_expr_new(db, TK_ID, &t_old, false);
> +		if (old_expr == NULL)
> +			pParse->is_aborted = true;
> +		expr = sql_expr_new(db, TK_ID, &t_to_col, false);
> +		if (expr == NULL)
> +			pParse->is_aborted = true;
> +		struct Expr *from_col_expr =
> +			sql_expr_new(db, TK_ID, &t_from_col, false);
> +		if (from_col_expr == NULL)
> +			pParse->is_aborted = true;
> +		struct Expr *to_col_expr =
> +			sqlPExpr(pParse, TK_DOT, old_expr, expr);
> +		struct Expr *eq =
> +			sqlPExpr(pParse, TK_EQ, to_col_expr, from_col_expr);
> +		if (where != NULL || eq != NULL) {
> +			where = sql_and_expr_new(db, where, eq);
> +			if (where == NULL)
> +				pParse->is_aborted = true;
> +		}

7. Talking of this hunk and the hunk below - I do not know
what to say. Even before the patch it looked cursed and corrupted,
but now it is even worse. Much worse. The code padded out in 2.5
times. Also, it is in fact repeated here and below. Fully repeated
with just different variable names. I've encapsulated most of
these into a new function sql_expr_new_2part_id() and batched
checks for expr == NULL to set aborts.

>   
>   		/*
>   		 * For ON UPDATE, construct the next term of the
> @@ -810,12 +839,22 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
>   		 *        no_action_needed(colN))
>   		 */
>   		if (is_update) {
> +			old_expr = sql_expr_new(db, TK_ID, &t_old, false);
> +			if (old_expr == NULL)
> +				pParse->is_aborted = true;
> +			expr = sql_expr_new(db, TK_ID, &t_to_col, false);
> +			if (expr == NULL)
> +				pParse->is_aborted = true;
>   			struct Expr *old_val = sqlPExpr(pParse, TK_DOT,
> -				sqlExprAlloc(db, TK_ID, &t_old, 0),
> -				sqlExprAlloc(db, TK_ID, &t_to_col, 0));
> +							old_expr, expr);
> +			new_expr = sql_expr_new(db, TK_ID, &t_new, false);
> +			if (new_expr == NULL)
> +				pParse->is_aborted = true;
> +			expr = sql_expr_new(db, TK_ID, &t_to_col, false);
> +			if (expr == NULL)
> +				pParse->is_aborted = true;
>   			struct Expr *new_val = sqlPExpr(pParse, TK_DOT,
> -				sqlExprAlloc(db, TK_ID, &t_new, 0),
> -				sqlExprAlloc(db, TK_ID, &t_to_col, 0));
> +							new_expr, expr);
>   			struct Expr *old_is_null = sqlPExpr(
>   				pParse, TK_ISNULL,
>   				sqlExprDup(db, old_val, 0), NULL);
> @@ -828,29 +867,41 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
>   		if (action != FKEY_ACTION_RESTRICT &&
>   		    (action != FKEY_ACTION_CASCADE || is_update)) {
>   			struct Expr *new, *d;
>   			if (action == FKEY_ACTION_CASCADE) {
> -				new = sqlPExpr(pParse, TK_DOT,
> -						   sqlExprAlloc(db, TK_ID,
> -								    &t_new, 0),
> -						   sqlExprAlloc(db, TK_ID,
> -								    &t_to_col,
> -								    0));
> +				new_expr = sql_expr_new(db, TK_ID, &t_new,
> +							false);
> +				if (new_expr == NULL)
> +					pParse->is_aborted = true;
> +				expr = sql_expr_new(db, TK_ID, &t_to_col,
> +						    false);
> +				if (expr == NULL)
> +					pParse->is_aborted = true;
> +				new = sqlPExpr(pParse, TK_DOT, new_expr, expr);
>   			} else if (action == FKEY_ACTION_SET_DEFAULT) {
>   				d = child_fields[chcol].default_value_expr;
>   				if (d != NULL) {
>   					new = sqlExprDup(db, d, 0);
>   				} else {
> -					new = sqlExprAlloc(db, TK_NULL,
> -							       NULL, 0);
> +					new = sql_expr_new(db, TK_NULL, NULL,
> +							   false);
> +					if (new == NULL)
> +						pParse->is_aborted = true;
>   				}
>   			} else {
> -				new = sqlExprAlloc(db, TK_NULL, NULL, 0);
> +				new = sql_expr_new(db, TK_NULL, NULL, false);
> +				if (new == NULL)
> +					pParse->is_aborted = true;
>   			}

8. Exactly the same about this code. Before the patch is was not the best,
but good enough because of low code duplication. But now the last branch
and the branch 'action == FKEY_ACTION_SET_DEFAULT && d == NULL' are too big
and still do the same. I've reworked that to remove the duplication.

>   			list = sql_expr_list_append(db, list, new);
>   			sqlExprListSetName(pParse, list, &t_from_col, 0);> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index f71990dee..4243e799c 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -898,15 +906,28 @@ term(A) ::= NULL(X).        {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/}
>  expr(A) ::= id(X).          {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/}
>  expr(A) ::= JOIN_KW(X).     {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/}
>  expr(A) ::= nm(X) DOT nm(Y). {
> -  Expr *temp1 = sqlExprAlloc(pParse->db, TK_ID, &X, 1);
> -  Expr *temp2 = sqlExprAlloc(pParse->db, TK_ID, &Y, 1);
> +  struct Expr *temp1 = sql_expr_new(pParse->db, TK_ID, &X, true);
> +  if (temp1 == NULL) {
> +    pParse->is_aborted = true;
> +    return;
> +  }
> +  struct Expr *temp2 = sql_expr_new(pParse->db, TK_ID, &Y, true);
> +  if (temp2 == NULL) {
> +    sql_expr_delete(pParse->db, temp2, false);

9. temp2 is NULL. You checked it one line above. It should be temp1.

> +    pParse->is_aborted = true;
> +    return;
> +  }> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 1b7d52b68..5bba0b5d0 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -497,8 +498,12 @@ addWhereTerm(Parse * pParse,	/* Parsing context */
>   	assert(pSrc->a[iLeft].space != NULL);
>   	assert(pSrc->a[iRight].space != NULL);
>   
> -	pE1 = sqlCreateColumnExpr(db, pSrc, iLeft, iColLeft);
> -	pE2 = sqlCreateColumnExpr(db, pSrc, iRight, iColRight);
> +	pE1 = sql_column_expr_new(pParse->db, pSrc, iLeft, iColLeft);
> +	if (pE1 == NULL)
> +		pParse->is_aborted = true;
> +	pE2 = sql_column_expr_new(pParse->db, pSrc, iRight, iColRight);
> +	if (pE2 == NULL)
> +		pParse->is_aborted = true;

10. You could check for both pE1 and pE2 == NULL in one 'if' using '||'.

>   
>   	pEq = sqlPExpr(pParse, TK_EQ, pE1, pE2);
>   	if (pEq && isOuterJoin) {
> @@ -628,8 +637,13 @@ sqlProcessJoin(Parse * pParse, Select * p)
>   		if (pRight->pOn) {
>   			if (isOuter)
>   				setJoinExpr(pRight->pOn, pRight->iCursor);
> -			p->pWhere =
> -			    sqlExprAnd(pParse->db, p->pWhere, pRight->pOn);
> +			if (p->pWhere != NULL || pRight->pOn != NULL) {

11. One another example why self-review is necessary. pRight->pOn is
already checked for not being equal NULL 2 lines above. It makes your
'if' useless.

> +				p->pWhere =
> +					sql_and_expr_new(pParse->db, p->pWhere,
> +							 pRight->pOn);
> +				if (p->pWhere == NULL)
> +					pParse->is_aborted = true;
> +			}
>   			pRight->pOn = 0;
>   		}
>   
> @@ -4324,8 +4346,13 @@ pushDownWhereTerms(Parse * pParse,	/* Parse context (for malloc() and error repo
>   		while (pSubq) {
>   			pNew = sqlExprDup(pParse->db, pWhere, 0);
>   			pNew = substExpr(pParse, pNew, iCursor, pSubq->pEList);
> -			pSubq->pWhere =
> -			    sqlExprAnd(pParse->db, pSubq->pWhere, pNew);
> +			if (pSubq->pWhere != NULL || pNew != NULL) {

12. If pNew is NULL, then it is an error, because pWhere is not NULL,
it is guaranteed by the code above (implicitly via attributes access,
for example). In such a case pNew NULL and pSubq->pWhere NULL mean
an error, and it is ok to set abort.

> +				pSubq->pWhere =
> +					sql_and_expr_new(pParse->db,
> +							 pSubq->pWhere, pNew);
> +				if (pSubq->pWhere == NULL)
> +					pParse->is_aborted = true;
> +			}
>   			pSubq = pSubq->pPrior;
>   		}
>   	}
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index d3ffa9d9b..13d0b16f8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3223,13 +3223,73 @@ void sqlClearTempRegCache(Parse *);
> +/**
> + * This routine is the core allocator for Expr nodes.
> + * Construct a new expression node and return a pointer to it.
> + * Memory for this node and for the token argument is a single
> + * allocation obtained from sqlDbMalloc(). The calling function
> + * is responsible for making sure the node eventually gets freed.
> + *
> + * If dequote is true, then the token (if it exists) is dequoted.
> + * If dequote is false, no dequoting is performed.  The dequote
> + * parameter is ignored if token is NULL or if the token does
> + * not appear to be quoted. If the quotes were of the form "..."
> + * (double-quotes) then the EP_DblQuoted flag is set on the
> + * expression node.
> + *
> + * Special case: If op==TK_INTEGER and token points to a string
> + * that can be translated into a 32-bit integer, then the token is
> + * not stored in u.zToken. Instead, the integer values is written
> + * into u.iValue and the EP_IntValue flag is set. No extra storage
> + * is allocated to hold the integer text and the dequote flag is
> + * ignored.
> + *
> + * @param db The database connection.
> + * @param op Expression opcode (TK_*).
> + * @param token Source token. Might be NULL.
> + * @param dequote True to dequote string.
> + * @retval Not NULL New expression object on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct Expr *
> +sql_expr_new(struct sql *db, int op, const Token *token, bool dequote);

13. After you has changed the last argument type to boolean, the
function call looks too long. I've split sql_expr_new into two
functions: sql_expr_new and sql_expr_new_dequoted.

> +
> +/**
> + * Allocate a new expression node from a zero-terminated token
> + * that has already been dequoted.
> + *
> + * @param db The database connection.
> + * @param op Expression opcode.
> + * @param name The object name string.
> + * @retval Not NULL Expression pointer on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct Expr *
> +sql_op_expr_new(struct sql *db, int op, const char *name);

14. That name looks strange. You want to emphasize that this
function takes an 'op'. But sql_expr_new() takes 'op' as well.
And what is more - uses it in the exactly same way. I've renamed
it to sql_expr_new_named(). Also, I've added an alias for NULL
names: sql_expr_new_anon().

> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index be30a40c0..036540d61 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -1372,7 +1372,13 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
>   					continue;
>   				testcase(pWC->a[iTerm].wtFlags & TERM_ORINFO);
>   				pExpr = sqlExprDup(db, pExpr, 0);
> -				pAndExpr = sqlExprAnd(db, pAndExpr, pExpr);
> +				if (pAndExpr != NULL || pExpr != NULL) {

15. If pExpr here is NULL, then it is already an error, because
2 lines above it was not NULL - it was used to check ExprHasProperty.
It means, that sql_and_expr_new() NULL result always is an error and it
is ok to abort.

> +					pAndExpr =
> +						sql_and_expr_new(db, pAndExpr,
> +								 pExpr);
> +					if (pAndExpr == NULL)
> +						pParse->is_aborted = true;
> +				}
>   			}
>   			if (pAndExpr) {
>   				pAndExpr =

==========================================================================

commit fc09568fad97d9bc548adaa9e54c8fcd7a88a980
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Tue Mar 26 11:38:07 2019 +0300

     Review fixes

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 10861dd15..2e4558408 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -616,7 +616,7 @@ sqlAddPrimaryKey(Parse * pParse,	/* Parsing context */
  		struct ExprList *list;
  		struct Token token;
  		sqlTokenInit(&token, space->def->fields[iCol].name);
-		struct Expr *expr = sql_expr_new(db, TK_ID, &token, false);
+		struct Expr *expr = sql_expr_new(db, TK_ID, &token);
  		if (expr == NULL) {
  			pParse->is_aborted = true;
  			goto primary_key_exit;
@@ -1358,12 +1358,13 @@ sql_id_eq_str_expr(struct Parse *parse, const char *col_name,
  		   const char *col_value)
  {
  	struct sql *db = parse->db;
-	struct Expr *col_name_expr = sql_op_expr_new(db, TK_ID, col_name);
+	struct Expr *col_name_expr = sql_expr_new_named(db, TK_ID, col_name);
  	if (col_name_expr == NULL) {
  		parse->is_aborted = true;
  		return NULL;
  	}
-	struct Expr *col_value_expr = sql_op_expr_new(db, TK_STRING, col_value);
+	struct Expr *col_value_expr =
+		sql_expr_new_named(db, TK_STRING, col_value);
  	if (col_value_expr == NULL) {
  		sql_expr_delete(db, col_name_expr, false);
  		parse->is_aborted = true;
@@ -1385,23 +1386,17 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
  		return;
  	}
  	src_list->a[0].zName = sqlDbStrDup(db, stat_table_name);
-	struct Expr *where = NULL;
+	struct Expr *expr, *where = NULL;
  	if (idx_name != NULL) {
-		struct Expr *expr = sql_id_eq_str_expr(parse, "idx", idx_name);
-		if (expr != NULL && (expr != NULL || where != NULL)) {
-			where = sql_and_expr_new(db, expr, where);
-			if (where == NULL)
-				parse->is_aborted = true;
-		}
+		expr = sql_id_eq_str_expr(parse, "idx", idx_name);
+		where = sql_and_expr_new(db, expr, where);
  	}
  	if (table_name != NULL) {
-		struct Expr *expr = sql_id_eq_str_expr(parse, "tbl", table_name);
-		if (expr != NULL && (expr != NULL || where != NULL)) {
-			where = sql_and_expr_new(db, expr, where);
-			if (where == NULL)
-				parse->is_aborted = true;
-		}
+		expr = sql_id_eq_str_expr(parse, "tbl", table_name);
+		where = sql_and_expr_new(db, expr, where);
  	}
+	if (where == NULL)
+		parse->is_aborted = true;
  	/**
  	 * On memory allocation error sql_table delete_from
  	 * releases memory for its own.
@@ -2256,12 +2251,12 @@ sql_create_index(struct Parse *parse, struct Token *token,
  		struct Token prev_col;
  		uint32_t last_field = def->field_count - 1;
  		sqlTokenInit(&prev_col, def->fields[last_field].name);
-		struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col, false);
+		struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col);
  		if (expr == NULL) {
  			parse->is_aborted = true;
  			goto exit_create_index;
  		}
-		col_list = sql_expr_list_append(parse->db, NULL, expr);
+		col_list = sql_expr_list_append(db, NULL, expr);
  		if (col_list == NULL)
  			goto exit_create_index;
  		assert(col_list->nExpr == 1);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index dad4ce3a6..9c8d9fdf4 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -151,8 +151,12 @@ sqlExprAddCollateToken(Parse * pParse,	/* Parsing context */
  {
  	if (pCollName->n == 0)
  		return pExpr;
-	struct Expr *new_expr =
-		sql_expr_new(pParse->db, TK_COLLATE, pCollName, dequote);
+	struct Expr *new_expr;
+	struct sql *db = pParse->db;
+	if (dequote)
+		new_expr = sql_expr_new_dequoted(db, TK_COLLATE, pCollName);
+	else
+		new_expr = sql_expr_new(db, TK_COLLATE, pCollName);
  	if (new_expr == NULL) {
  		pParse->is_aborted = true;
  		return pExpr;
@@ -876,7 +880,7 @@ sqlExprSetHeightAndFlags(Parse * pParse, Expr * p)
  #endif				/* SQL_MAX_EXPR_DEPTH>0 */
  
  struct Expr *
-sql_expr_new(struct sql *db, int op, const Token *token, bool dequote)
+sql_expr_new(struct sql *db, int op, const struct Token *token)
  {
  	int extra_sz = 0;
  	int val = 0;
@@ -911,25 +915,23 @@ sql_expr_new(struct sql *db, int op, const Token *token, bool dequote)
  		assert(token->z != NULL || token->n == 0);
  		memcpy(expr->u.zToken, token->z, token->n);
  		expr->u.zToken[token->n] = '\0';
-		if (dequote) {
-			if (expr->u.zToken[0] == '"')
-				expr->flags |= EP_DblQuoted;
-			if (expr->op == TK_ID || expr->op == TK_COLLATE ||
-			    expr->op == TK_FUNCTION)
-				sqlNormalizeName(expr->u.zToken);
-			else
-				sqlDequote(expr->u.zToken);
-		}
  	}
  	return expr;
  }
  
  struct Expr *
-sql_op_expr_new(struct sql *db, int op, const char *name)
+sql_expr_new_dequoted(struct sql *db, int op, const struct Token *token)
  {
-	struct Token name_token;
-	sqlTokenInit(&name_token, (char *)name);
-	return sql_expr_new(db, op, &name_token, false);
+	struct Expr *e = sql_expr_new(db, op, token);
+	if (e == NULL || (e->flags & EP_IntValue) != 0 || e->u.zToken == NULL)
+		return e;
+	if (e->u.zToken[0] == '"')
+		e->flags |= EP_DblQuoted;
+	if (e->op == TK_ID || e->op == TK_COLLATE || e->op == TK_FUNCTION)
+		sqlNormalizeName(e->u.zToken);
+	else
+		sqlDequote(e->u.zToken);
+	return e;
  }
  
  /*
@@ -979,8 +981,6 @@ sqlPExpr(Parse * pParse,	/* Parsing context */
  		 * Take advantage of short-circuit false
  		 * optimization for AND.
  		 */
-		if (pLeft == NULL && pRight == NULL)
-			return NULL;
  		p = sql_and_expr_new(pParse->db, pLeft, pRight);
  		if (p == NULL)
  			pParse->is_aborted = true;
@@ -1057,17 +1057,15 @@ sql_and_expr_new(struct sql *db, struct Expr *left_expr,
  		 struct Expr *right_expr)
  {
  	if (left_expr == NULL) {
-		assert(right_expr != NULL);
  		return right_expr;
  	} else if (right_expr == NULL) {
-		assert(left_expr != NULL);
  		return left_expr;
  	} else if (exprAlwaysFalse(left_expr) || exprAlwaysFalse(right_expr)) {
  		sql_expr_delete(db, left_expr, false);
  		sql_expr_delete(db, right_expr, false);
-		return sql_expr_new(db, TK_INTEGER, &sqlIntTokens[0], false);
+		return sql_expr_new(db, TK_INTEGER, &sqlIntTokens[0]);
  	} else {
-		struct Expr *new_expr = sql_expr_new(db, TK_AND, NULL, false);
+		struct Expr *new_expr = sql_expr_new_anon(db, TK_AND);
  		sqlExprAttachSubtrees(db, new_expr, left_expr, right_expr);
  		return new_expr;
  	}
@@ -1082,7 +1080,7 @@ sqlExprFunction(Parse * pParse, ExprList * pList, Token * pToken)
  {
  	struct sql *db = pParse->db;
  	assert(pToken != NULL);
-	struct Expr *new_expr = sql_expr_new(db, TK_FUNCTION, pToken, true);
+	struct Expr *new_expr = sql_expr_new_dequoted(db, TK_FUNCTION, pToken);
  	if (new_expr == NULL) {
  		sql_expr_list_delete(db, pList);
  		pParse->is_aborted = true;
@@ -2903,7 +2901,7 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
  			if (pSel->pLimit == NULL) {
  				pSel->pLimit =
  					sql_expr_new(pParse->db, TK_INTEGER,
-						     &sqlIntTokens[1], false);
+						     &sqlIntTokens[1]);
  				if (pSel->pLimit == NULL) {
  					pParse->is_aborted = true;
  				} else {
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 3b27b2d4f..8151c66e5 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -302,36 +302,28 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
  }
  
  /**
- * Return an Expr object that refers to a memory register
- * corresponding to column of given space.
- *
- * reg_base is the first of an array of register that contains
- * the data for given space. reg_base+1 holds the first column.
- * reg_base+2 holds the second column, and so forth.
- *
- * @param parser The parsing context.
- * @param def Definition of space whose content is at r[regBase]...
- * @param reg_base Contents of table table.
- * @param column Index of table column is desired.
- * @retval Not NULL expression pointer on success.
- * @retval NULL Otherwise.
+ * Build an expression that refers to a memory register
+ * corresponding to @a column of given space.
+ *
+ * @param db SQL context.
+ * @param def Definition of space whose content starts from
+ *        @a reg_base register.
+ * @param reg_base Index of a first element in an array of
+ *        registers, containing data of a space. Register
+ *        reg_base + i holds an i-th column, i >= 1.
+ * @param column Index of a first table column to point at.
+ * @retval Not NULL Success. An expression representing register.
+ * @retval NULL Error. A diag message is set.
   */
  static struct Expr *
-sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base,
-		      int column)
+sql_expr_new_register(struct sql *db, struct space_def *def, int reg_base,
+		      uint32_t column)
  {
-	struct Expr *expr = sql_op_expr_new(parser->db, TK_REGISTER, NULL);
-	if (expr == NULL) {
-		parser->is_aborted = true;
+	struct Expr *expr = sql_expr_new_anon(db, TK_REGISTER);
+	if (expr == NULL)
  		return NULL;
-	}
-	if (column >= 0) {
-		expr->iTable = reg_base + column + 1;
-		expr->type = def->fields[column].type;
-	} else {
-		expr->iTable = reg_base;
-		expr->type = FIELD_TYPE_INTEGER;
-	}
+	expr->iTable = reg_base + column + 1;
+	expr->type = def->fields[column].type;
  	return expr;
  }
  
@@ -346,10 +338,10 @@ sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base,
   * @retval NULL on error.
   */
  static struct Expr *
-sql_column_cursor_expr_create(struct sql *db, struct space_def *def,
+sql_expr_new_column_by_cursor(struct sql *db, struct space_def *def,
  			      int cursor, int column)
  {
-	struct Expr *expr = sql_op_expr_new(db, TK_COLUMN, NULL);
+	struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN);
  	if (expr == NULL)
  		return NULL;
  	expr->space_def = def;
@@ -435,18 +427,14 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src,
  	for (uint32_t i = 0; i < fk_def->field_count; i++) {
  		uint32_t fieldno = fk_def->links[i].parent_field;
  		struct Expr *pexpr =
-			sql_register_expr_new(parser, def, reg_data, fieldno);
+			sql_expr_new_register(db, def, reg_data, fieldno);
  		fieldno = fk_def->links[i].child_field;
  		const char *field_name = child_space->def->fields[fieldno].name;
-		struct Expr *chexpr = sql_op_expr_new(db, TK_ID, field_name);
-		if (chexpr == NULL)
-			parser->is_aborted = true;
+		struct Expr *chexpr = sql_expr_new_named(db, TK_ID, field_name);
  		struct Expr *eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr);
-		if (where != NULL || eq != NULL) {
-			where = sql_and_expr_new(db, where, eq);
-			if (where == NULL)
-				parser->is_aborted = true;
-		}
+		where = sql_and_expr_new(db, where, eq);
+		if (where == NULL || chexpr == NULL || pexpr == NULL)
+			parser->is_aborted = true;
  	}
  
  	/*
@@ -462,26 +450,20 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src,
  		struct Expr *expr = NULL, *pexpr, *chexpr, *eq;
  		for (uint32_t i = 0; i < fk_def->field_count; i++) {
  			uint32_t fieldno = fk_def->links[i].parent_field;
-			pexpr = sql_register_expr_new(parser, def, reg_data,
+			pexpr = sql_expr_new_register(db, def, reg_data,
  						      fieldno);
  			int cursor = src->a[0].iCursor;
-			chexpr = sql_column_cursor_expr_create(db, def, cursor,
+			chexpr = sql_expr_new_column_by_cursor(db, def, cursor,
  							       fieldno);
-			if (chexpr == NULL)
-				parser->is_aborted = true;
  			eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr);
-			if (expr != NULL || eq != NULL) {
-				expr = sql_and_expr_new(db, expr, eq);
-				if (expr == NULL)
-					parser->is_aborted = true;
-			}
-		}
-		struct Expr *pNe = sqlPExpr(parser, TK_NOT, expr, 0);
-		if (where != NULL || pNe != NULL) {
-			where = sql_and_expr_new(db, where, pNe);
-			if (where == NULL)
+			expr = sql_and_expr_new(db, expr, eq);
+			if (expr == NULL || chexpr == NULL || pexpr == NULL)
  				parser->is_aborted = true;
  		}
+		struct Expr *pNe = sqlPExpr(parser, TK_NOT, expr, 0);
+		where = sql_and_expr_new(db, where, pNe);
+		if (where == NULL)
+			parser->is_aborted = true;
  	}
  
  	/* Resolve the references in the WHERE clause. */
@@ -720,6 +702,28 @@ fk_constraint_is_required(struct space *space, const int *changes)
  	return false;
  }
  
+/**
+ * Create a new expression representing two-part path
+ * '<main>.<sub>'.
+ * @param parser SQL Parser.
+ * @param main First and main part of a result path. For example,
+ *        a table name.
+ * @param sub Second and last part of a result path. For example,
+ *        a column name.
+ * @retval Not NULL Success. An expression with two-part path.
+ * @retval NULL Error. A diag message is set.
+ */
+static inline struct Expr *
+sql_expr_new_2part_id(struct Parse *parser, const struct Token *main,
+		      const struct Token *sub)
+{
+	struct Expr *emain = sql_expr_new(parser->db, TK_ID, main);
+	struct Expr *esub = sql_expr_new(parser->db, TK_ID, sub);
+	if (emain == NULL || esub == NULL)
+		parser->is_aborted = true;
+	return sqlPExpr(parser, TK_DOT, emain, esub);
+}
+
  /**
   * This function is called when an UPDATE or DELETE operation is
   * being compiled on table pTab, which is the parent table of
@@ -802,27 +806,13 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
  		 * type and collation sequence associated with
  		 * the parent table are used for the comparison.
  		 */
-		struct Expr *old_expr = NULL, *new_expr = NULL, *expr = NULL;
-		old_expr = sql_expr_new(db, TK_ID, &t_old, false);
-		if (old_expr == NULL)
-			pParse->is_aborted = true;
-		expr = sql_expr_new(db, TK_ID, &t_to_col, false);
-		if (expr == NULL)
-			pParse->is_aborted = true;
-		struct Expr *from_col_expr =
-			sql_expr_new(db, TK_ID, &t_from_col, false);
-		if (from_col_expr == NULL)
+		struct Expr *new, *old =
+			sql_expr_new_2part_id(pParse, &t_old, &t_to_col);
+		struct Expr *from = sql_expr_new(db, TK_ID, &t_from_col);
+		struct Expr *eq = sqlPExpr(pParse, TK_EQ, old, from);
+		where = sql_and_expr_new(db, where, eq);
+		if (where == NULL || from == NULL)
  			pParse->is_aborted = true;
-		struct Expr *to_col_expr =
-			sqlPExpr(pParse, TK_DOT, old_expr, expr);
-		struct Expr *eq =
-			sqlPExpr(pParse, TK_EQ, to_col_expr, from_col_expr);
-		if (where != NULL || eq != NULL) {
-			where = sql_and_expr_new(db, where, eq);
-			if (where == NULL)
-				pParse->is_aborted = true;
-		}
-
  		/*
  		 * For ON UPDATE, construct the next term of the
  		 * WHEN clause, which should return false in case
@@ -839,67 +829,36 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
  		 *        no_action_needed(colN))
  		 */
  		if (is_update) {
-			old_expr = sql_expr_new(db, TK_ID, &t_old, false);
-			if (old_expr == NULL)
-				pParse->is_aborted = true;
-			expr = sql_expr_new(db, TK_ID, &t_to_col, false);
-			if (expr == NULL)
-				pParse->is_aborted = true;
-			struct Expr *old_val = sqlPExpr(pParse, TK_DOT,
-							old_expr, expr);
-			new_expr = sql_expr_new(db, TK_ID, &t_new, false);
-			if (new_expr == NULL)
-				pParse->is_aborted = true;
-			expr = sql_expr_new(db, TK_ID, &t_to_col, false);
-			if (expr == NULL)
-				pParse->is_aborted = true;
-			struct Expr *new_val = sqlPExpr(pParse, TK_DOT,
-							new_expr, expr);
-			struct Expr *old_is_null = sqlPExpr(
-				pParse, TK_ISNULL,
-				sqlExprDup(db, old_val, 0), NULL);
-			eq = sqlPExpr(pParse, TK_EQ, old_val,
-				sqlExprDup(db, new_val, 0));
+			old = sql_expr_new_2part_id(pParse, &t_old, &t_to_col);
+			new = sql_expr_new_2part_id(pParse, &t_new, &t_to_col);
+			struct Expr *old_is_null =
+				sqlPExpr(pParse, TK_ISNULL,
+					 sqlExprDup(db, old, 0), NULL);
+			eq = sqlPExpr(pParse, TK_EQ, old,
+				      sqlExprDup(db, new, 0));
  			struct Expr *new_non_null =
-				sqlPExpr(pParse, TK_NOTNULL, new_val, NULL);
+				sqlPExpr(pParse, TK_NOTNULL, new, NULL);
  			struct Expr *non_null_eq =
  				sqlPExpr(pParse, TK_AND, new_non_null, eq);
  			struct Expr *no_action_needed =
  				sqlPExpr(pParse, TK_OR, old_is_null,
  					     non_null_eq);
-			if (when != NULL || no_action_needed != NULL) {
-				when = sql_and_expr_new(db, when,
-							no_action_needed);
-				if (when == NULL)
-					pParse->is_aborted = true;
-			}
+			when = sql_and_expr_new(db, when, no_action_needed);
+			if (when == NULL)
+				pParse->is_aborted = true;
  		}
  
  		if (action != FKEY_ACTION_RESTRICT &&
  		    (action != FKEY_ACTION_CASCADE || is_update)) {
-			struct Expr *new, *d;
+			struct Expr *d = child_fields[chcol].default_value_expr;
  			if (action == FKEY_ACTION_CASCADE) {
-				new_expr = sql_expr_new(db, TK_ID, &t_new,
-							false);
-				if (new_expr == NULL)
-					pParse->is_aborted = true;
-				expr = sql_expr_new(db, TK_ID, &t_to_col,
-						    false);
-				if (expr == NULL)
-					pParse->is_aborted = true;
-				new = sqlPExpr(pParse, TK_DOT, new_expr, expr);
-			} else if (action == FKEY_ACTION_SET_DEFAULT) {
-				d = child_fields[chcol].default_value_expr;
-				if (d != NULL) {
-					new = sqlExprDup(db, d, 0);
-				} else {
-					new = sql_expr_new(db, TK_NULL, NULL,
-							   false);
-					if (new == NULL)
-						pParse->is_aborted = true;
-				}
+				new = sql_expr_new_2part_id(pParse, &t_new,
+							    &t_to_col);
+			} else if (action == FKEY_ACTION_SET_DEFAULT &&
+				   d != NULL) {
+				new = sqlExprDup(db, d, 0);
  			} else {
-				new = sql_expr_new(db, TK_NULL, NULL, false);
+				new = sql_expr_new_anon(db, TK_NULL);
  				if (new == NULL)
  					pParse->is_aborted = true;
  			}
@@ -915,9 +874,8 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def,
  		struct Token err;
  		err.z = space_name;
  		err.n = name_len;
-		struct Expr *r =
-			sql_op_expr_new(db, TK_RAISE,
-					"FOREIGN KEY constraint failed");
+		struct Expr *r = sql_expr_new_named(db, TK_RAISE, "FOREIGN "\
+						    "KEY constraint failed");
  		if (r == NULL)
  			pParse->is_aborted = true;
  		else
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 4243e799c..c749cf1bd 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -536,7 +536,7 @@ selcollist(A) ::= sclp(A) expr(X) as(Y).     {
     sqlExprListSetSpan(pParse,A,&X);
  }
  selcollist(A) ::= sclp(A) STAR. {
-  struct Expr *p = sql_op_expr_new(pParse->db, TK_ASTERISK, NULL);
+  struct Expr *p = sql_expr_new_anon(pParse->db, TK_ASTERISK);
    if (p == NULL) {
      pParse->is_aborted = true;
      return;
@@ -544,7 +544,7 @@ selcollist(A) ::= sclp(A) STAR. {
    A = sql_expr_list_append(pParse->db, A, p);
  }
  selcollist(A) ::= sclp(A) nm(X) DOT STAR. {
-  struct Expr *pLeft = sql_expr_new(pParse->db, TK_ID, &X, true);
+  struct Expr *pLeft = sql_expr_new_dequoted(pParse->db, TK_ID, &X);
    if (pLeft == NULL) {
      pParse->is_aborted = true;
      return;
@@ -906,14 +906,14 @@ term(A) ::= NULL(X).        {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/}
  expr(A) ::= id(X).          {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/}
  expr(A) ::= JOIN_KW(X).     {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/}
  expr(A) ::= nm(X) DOT nm(Y). {
-  struct Expr *temp1 = sql_expr_new(pParse->db, TK_ID, &X, true);
+  struct Expr *temp1 = sql_expr_new_dequoted(pParse->db, TK_ID, &X);
    if (temp1 == NULL) {
      pParse->is_aborted = true;
      return;
    }
-  struct Expr *temp2 = sql_expr_new(pParse->db, TK_ID, &Y, true);
+  struct Expr *temp2 = sql_expr_new_dequoted(pParse->db, TK_ID, &Y);
    if (temp2 == NULL) {
-    sql_expr_delete(pParse->db, temp2, false);
+    sql_expr_delete(pParse->db, temp1, false);
      pParse->is_aborted = true;
      return;
    }
@@ -923,7 +923,7 @@ expr(A) ::= nm(X) DOT nm(Y). {
  term(A) ::= FLOAT|BLOB(X). {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/}
  term(A) ::= STRING(X).     {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/}
  term(A) ::= INTEGER(X). {
-  A.pExpr = sql_expr_new(pParse->db, TK_INTEGER, &X, true);
+  A.pExpr = sql_expr_new_dequoted(pParse->db, TK_INTEGER, &X);
    if (A.pExpr == NULL) {
      pParse->is_aborted = true;
      return;
@@ -963,7 +963,7 @@ expr(A) ::= expr(A) COLLATE id(C). {
  %ifndef SQL_OMIT_CAST
  expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
    spanSet(&A,&X,&Y); /*A-overwrites-X*/
-  A.pExpr = sql_expr_new(pParse->db, TK_CAST, NULL, true);
+  A.pExpr = sql_expr_new_dequoted(pParse->db, TK_CAST, NULL);
    if (A.pExpr == NULL) {
      pParse->is_aborted = true;
      return;
@@ -1161,7 +1161,7 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] {
      ** regardless of the value of expr1.
      */
      sql_expr_delete(pParse->db, A.pExpr, false);
-    A.pExpr = sql_expr_new(pParse->db, TK_INTEGER, &sqlIntTokens[N], true);
+    A.pExpr = sql_expr_new_dequoted(pParse->db, TK_INTEGER, &sqlIntTokens[N]);
      if (A.pExpr == NULL) {
        pParse->is_aborted = true;
        return;
@@ -1506,7 +1506,7 @@ expr(A) ::= RAISE(X) LP IGNORE RP(Y).  {
  }
  expr(A) ::= RAISE(X) LP raisetype(T) COMMA STRING(Z) RP(Y).  {
    spanSet(&A,&X,&Y);  /*A-overwrites-X*/
-  A.pExpr = sql_expr_new(pParse->db, TK_RAISE, &Z, true);
+  A.pExpr = sql_expr_new_dequoted(pParse->db, TK_RAISE, &Z);
    if(A.pExpr == NULL) {
      pParse->is_aborted = true;
      return;
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index eb74d024c..075e1fab4 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -488,10 +488,10 @@ lookupName(Parse * pParse,	/* The parsing context */
  }
  
  struct Expr *
-sql_column_expr_new(struct sql *db, struct SrcList *src_list, int src_idx,
+sql_expr_new_column(struct sql *db, struct SrcList *src_list, int src_idx,
  		    int column)
  {
-	struct Expr *expr = sql_expr_new(db, TK_COLUMN, NULL, false);
+	struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN);
  	if (expr == NULL)
  		return NULL;
  	struct SrcList_item *item = &src_list->a[src_idx];
@@ -995,7 +995,7 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
  				 * taking care to preserve the COLLATE clause if it exists
  				 */
  				struct Expr *pNew =
-					sql_op_expr_new(db, TK_INTEGER, NULL);
+					sql_expr_new_anon(db, TK_INTEGER);
  				if (pNew == NULL) {
  					pParse->is_aborted = true;
  					return 1;
@@ -1349,7 +1349,7 @@ resolveSelectStep(Walker * pWalker, Select * p)
  			 */
  			sql_expr_delete(db, p->pLimit, false);
  			p->pLimit = sql_expr_new(db, TK_INTEGER,
-						 &sqlIntTokens[1], false);
+						 &sqlIntTokens[1]);
  			if (p->pLimit == NULL)
  				pParse->is_aborted = true;
  		} else {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 5bba0b5d0..6d115182d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -164,10 +164,10 @@ sqlSelectNew(Parse * pParse,	/* Parsing context */
  		pNew = &standin;
  	}
  	if (pEList == 0) {
-		struct Expr *expr = sql_op_expr_new(db, TK_ASTERISK, NULL);
+		struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
  		if (expr == NULL)
  			pParse->is_aborted = true;
-		pEList = sql_expr_list_append(pParse->db, NULL, expr);
+		pEList = sql_expr_list_append(db, NULL, expr);
  	}
  	struct session MAYBE_UNUSED *user_session;
  	user_session = current_session();
@@ -489,8 +489,7 @@ addWhereTerm(Parse * pParse,	/* Parsing context */
  	     int isOuterJoin,	/* True if this is an OUTER join */
  	     Expr ** ppWhere)	/* IN/OUT: The WHERE clause to add to */
  {
-	Expr *pE1;
-	Expr *pE2;
+	struct sql *db = pParse->db;
  	Expr *pEq;
  
  	assert(iLeft < iRight);
@@ -498,13 +497,10 @@ addWhereTerm(Parse * pParse,	/* Parsing context */
  	assert(pSrc->a[iLeft].space != NULL);
  	assert(pSrc->a[iRight].space != NULL);
  
-	pE1 = sql_column_expr_new(pParse->db, pSrc, iLeft, iColLeft);
-	if (pE1 == NULL)
+	struct Expr *pE1 = sql_expr_new_column(db, pSrc, iLeft, iColLeft);
+	struct Expr *pE2 = sql_expr_new_column(db, pSrc, iRight, iColRight);
+	if (pE1 == NULL || pE2 == NULL)
  		pParse->is_aborted = true;
-	pE2 = sql_column_expr_new(pParse->db, pSrc, iRight, iColRight);
-	if (pE2 == NULL)
-		pParse->is_aborted = true;
-
  	pEq = sqlPExpr(pParse, TK_EQ, pE1, pE2);
  	if (pEq && isOuterJoin) {
  		ExprSetProperty(pEq, EP_FromJoin);
@@ -512,11 +508,9 @@ addWhereTerm(Parse * pParse,	/* Parsing context */
  		ExprSetVVAProperty(pEq, EP_NoReduce);
  		pEq->iRightJoinTable = (i16) pE2->iTable;
  	}
-	if (*ppWhere != NULL || pEq != NULL) {
-		*ppWhere = sql_and_expr_new(pParse->db, *ppWhere, pEq);
-		if (*ppWhere == NULL)
-			pParse->is_aborted = true;
-	}
+	*ppWhere = sql_and_expr_new(db, *ppWhere, pEq);
+	if (*ppWhere == NULL)
+		pParse->is_aborted = true;
  }
  
  /*
@@ -637,13 +631,10 @@ sqlProcessJoin(Parse * pParse, Select * p)
  		if (pRight->pOn) {
  			if (isOuter)
  				setJoinExpr(pRight->pOn, pRight->iCursor);
-			if (p->pWhere != NULL || pRight->pOn != NULL) {
-				p->pWhere =
-					sql_and_expr_new(pParse->db, p->pWhere,
-							 pRight->pOn);
-				if (p->pWhere == NULL)
-					pParse->is_aborted = true;
-			}
+			p->pWhere = sql_and_expr_new(pParse->db, p->pWhere,
+						     pRight->pOn);
+			if (p->pWhere == NULL)
+				pParse->is_aborted = true;
  			pRight->pOn = 0;
  		}
  
@@ -3349,10 +3340,10 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
  			}
  			if (j == nOrderBy) {
  				struct Expr *pNew =
-					sql_op_expr_new(db, TK_INTEGER, NULL);
+					sql_expr_new_anon(db, TK_INTEGER);
  				if (pNew == NULL) {
  					pParse->is_aborted = true;
-					return SQL_NOMEM;
+					return 1;
  				}
  				pNew->flags |= EP_IntValue;
  				pNew->u.iValue = i;
@@ -4226,7 +4217,8 @@ flattenSubquery(Parse * pParse,		/* Parsing context */
  			assert(pParent->pHaving == 0);
  			pParent->pHaving = pParent->pWhere;
  			pParent->pWhere = pWhere;
-			struct Expr *sub_having = sqlExprDup(db, pSub->pHaving, 0);
+			struct Expr *sub_having =
+				sqlExprDup(db, pSub->pHaving, 0);
  			if (sub_having != NULL || pParent->pHaving != NULL) {
  				pParent->pHaving =
  					sql_and_expr_new(db, sub_having,
@@ -4346,13 +4338,10 @@ pushDownWhereTerms(Parse * pParse,	/* Parse context (for malloc() and error repo
  		while (pSubq) {
  			pNew = sqlExprDup(pParse->db, pWhere, 0);
  			pNew = substExpr(pParse, pNew, iCursor, pSubq->pEList);
-			if (pSubq->pWhere != NULL || pNew != NULL) {
-				pSubq->pWhere =
-					sql_and_expr_new(pParse->db,
+			pSubq->pWhere = sql_and_expr_new(pParse->db,
  							 pSubq->pWhere, pNew);
-				if (pSubq->pWhere == NULL)
-					pParse->is_aborted = true;
-			}
+			if (pSubq->pWhere == NULL)
+				pParse->is_aborted = true;
  			pSubq = pSubq->pPrior;
  		}
  	}
@@ -4533,7 +4522,7 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p)
  		return WRC_Abort;
  	*pNew = *p;
  	p->pSrc = pNewSrc;
-	struct Expr *expr = sql_op_expr_new(db, TK_ASTERISK, NULL);
+	struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK);
  	if (expr == NULL)
  		pParse->is_aborted = true;
  	p->pEList = sql_expr_list_append(pParse->db, NULL, expr);
@@ -5020,7 +5009,7 @@ selectExpander(Walker * pWalker, Select * p)
  								continue;
  							}
  						}
-						pRight = sql_op_expr_new(db,
+						pRight = sql_expr_new_named(db,
  								TK_ID, zName);
  						if (pRight == NULL)
  							pParse->is_aborted = true;
@@ -5029,7 +5018,7 @@ selectExpander(Walker * pWalker, Select * p)
  						if (longNames
  						    || pTabList->nSrc > 1) {
  							Expr *pLeft;
-							pLeft = sql_op_expr_new(
+							pLeft = sql_expr_new_named(
  									db,
  									TK_ID,
  									zTabName);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 5a7a67f2d..7182f58fd 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3225,48 +3225,54 @@ int sqlNoTempsInRange(Parse *, int, int);
  #endif
  
  /**
- * This routine is the core allocator for Expr nodes.
- * Construct a new expression node and return a pointer to it.
- * Memory for this node and for the token argument is a single
- * allocation obtained from sqlDbMalloc(). The calling function
- * is responsible for making sure the node eventually gets freed.
- *
- * If dequote is true, then the token (if it exists) is dequoted.
- * If dequote is false, no dequoting is performed.  The dequote
- * parameter is ignored if token is NULL or if the token does
- * not appear to be quoted. If the quotes were of the form "..."
- * (double-quotes) then the EP_DblQuoted flag is set on the
- * expression node.
+ * Construct a new expression. Memory for this node and for the
+ * token argument is a single allocation obtained from
+ * sqlDbMalloc(). The calling function is responsible for making
+ * sure the node eventually gets freed.
   *
   * Special case: If op==TK_INTEGER and token points to a string
   * that can be translated into a 32-bit integer, then the token is
   * not stored in u.zToken. Instead, the integer values is written
   * into u.iValue and the EP_IntValue flag is set. No extra storage
- * is allocated to hold the integer text and the dequote flag is
- * ignored.
+ * is allocated to hold the integer text.
   *
   * @param db The database connection.
   * @param op Expression opcode (TK_*).
   * @param token Source token. Might be NULL.
- * @param dequote True to dequote string.
   * @retval Not NULL New expression object on success.
   * @retval NULL Otherwise. The diag message is set.
   */
  struct Expr *
-sql_expr_new(struct sql *db, int op, const Token *token, bool dequote);
+sql_expr_new(struct sql *db, int op, const struct Token *token);
  
  /**
- * Allocate a new expression node from a zero-terminated token
- * that has already been dequoted.
- *
- * @param db The database connection.
- * @param op Expression opcode.
- * @param name The object name string.
- * @retval Not NULL Expression pointer on success.
- * @retval NULL Otherwise. The diag message is set.
+ * The same as @sa sql_expr_new, but normalizes name, stored in
+ * @a token. Quotes are removed if they are presented.
   */
  struct Expr *
-sql_op_expr_new(struct sql *db, int op, const char *name);
+sql_expr_new_dequoted(struct sql *db, int op, const struct Token *token);
+
+/**
+ * The same as @a sql_expr_new, but takes const char instead of
+ * Token. Just sugar to do not touch tokens in many places.
+ */
+static inline struct Expr *
+sql_expr_new_named(struct sql *db, int op, const char *name)
+{
+	struct Token name_token;
+	sqlTokenInit(&name_token, (char *)name);
+	return sql_expr_new(db, op, &name_token);
+}
+
+/**
+ * The same as @a sql_expr_new, but a result expression has no
+ * name.
+ */
+static inline struct Expr *
+sql_expr_new_anon(struct sql *db, int op)
+{
+	return sql_expr_new_named(db, op, NULL);
+}
  
  void sqlExprAttachSubtrees(sql *, Expr *, Expr *, Expr *);
  Expr *sqlPExpr(Parse *, int, Expr *, Expr *);
@@ -3284,7 +3290,8 @@ void sqlPExprAddSelect(Parse *, Expr *, Select *);
   * @param left_expr The left-branch expresion to join.
   * @param right_expr The right-branch expression to join.
   * @retval Not NULL New expression root node pointer on success.
- * @retval NULL Otherwise. The diag message is set.
+ * @retval NULL Error. A diag message is set.
+ * @retval NULL Not an error. Both arguments were NULL.
   */
  struct Expr *
  sql_and_expr_new(struct sql *db, struct Expr *left_expr,
@@ -4810,18 +4817,18 @@ void sqlStrAccumReset(StrAccum *);
  void sqlSelectDestInit(SelectDest *, int, int, int);
  
  /*
- * Allocate and return a pointer to an expression to load the
- * column from datasource src_idx in SrcList src_list.
+ * Create an expression to load @a column from datasource
+ * @a src_idx in @a src_list.
   *
   * @param db The database connection.
   * @param src_list The source list described with FROM clause.
   * @param src_idx The resource index to use in src_list.
   * @param column The column index.
- * @retval Not NULL Expression pointer on success.
- * @retval NULL Otherwise. The diag message is set.
+ * @retval Not NULL Success. An expression to load @a column.
+ * @retval NULL Error. A diag message is set.
   */
  struct Expr *
-sql_column_expr_new(struct sql *db, struct SrcList *src_list, int src_idx,
+sql_expr_new_column(struct sql *db, struct SrcList *src_list, int src_idx,
  		    int column);
  
  int sqlExprCheckIN(Parse *, Expr *);
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 036540d61..a453fe979 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1372,13 +1372,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
  					continue;
  				testcase(pWC->a[iTerm].wtFlags & TERM_ORINFO);
  				pExpr = sqlExprDup(db, pExpr, 0);
-				if (pAndExpr != NULL || pExpr != NULL) {
-					pAndExpr =
-						sql_and_expr_new(db, pAndExpr,
-								 pExpr);
-					if (pAndExpr == NULL)
-						pParse->is_aborted = true;
-				}
+				pAndExpr = sql_and_expr_new(db, pAndExpr,
+							    pExpr);
+				if (pAndExpr == NULL)
+					pParse->is_aborted = true;
  			}
  			if (pAndExpr) {
  				pAndExpr =
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index a9dec8e99..d22a4e0f4 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -307,7 +307,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
  			Expr *pPrefix;
  			*pisComplete = c == MATCH_ALL_WILDCARD &&
  				       z[cnt + 1] == 0;
-			pPrefix = sql_op_expr_new(db, TK_STRING, z);
+			pPrefix = sql_expr_new_named(db, TK_STRING, z);
  			if (pPrefix == NULL)
  				pParse->is_aborted = true;
  			else
@@ -1308,7 +1308,7 @@ exprAnalyze(SrcList * pSrc,	/* the FROM clause */
  		Expr *pLeft = pExpr->pLeft;
  		int idxNew;
  		WhereTerm *pNewTerm;
-		struct Expr *expr = sql_expr_new(db, TK_NULL, NULL, false);
+		struct Expr *expr = sql_expr_new_anon(db, TK_NULL);
  		if (expr == NULL)
  			pParse->is_aborted = true;
  		pNewExpr = sqlPExpr(pParse, TK_GT, sqlExprDup(db, pLeft, 0),
@@ -1505,7 +1505,7 @@ sqlWhereTabFuncArgs(Parse * pParse,	/* Parsing context */
  					space_def->name, j);
  			return;
  		}
-		pColRef = sql_expr_new(pParse->db, TK_COLUMN, NULL, false);
+		pColRef = sql_expr_new_anon(pParse->db, TK_COLUMN);
  		if (pColRef == NULL) {
  			pParse->is_aborted = true;
  			return;