[tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()

["n.pettik" <korablev@tarantool.org>]

> @@ -1284,10 +1302,13 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
> 		goto create_view_fail;
> 	if (aliases != NULL) {
> 		if ((int)select_res_space->def->field_count != aliases->nExpr) {
> -			sqlErrorMsg(parse_context, "expected %d columns "\
> -					"for '%s' but got %d", aliases->nExpr,
> -					space->def->name,
> -					select_res_space->def->field_count);
> +			const char *err_msg =
> +				tt_sprintf("expected %d columns for '%s' but "\
> +					   "got %d", aliases->nExpr,
> +					   space->def->name,
> +					   select_res_space->def->field_count);
> +			diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +			parse_context->is_aborted = true;
> 			goto create_view_fail;
> 		}
> 		sqlColumnsFromExprList(parse_context, aliases, space->def);
> @@ -1609,13 +1630,17 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
> 	 * and DROP VIEW is not used on a table.
> 	 */
> 	if (is_view && !space->def->opts.is_view) {
> -		sqlErrorMsg(parse_context, "use DROP TABLE to delete table %s",
> -				space_name);
> +		const char *err_msg = tt_sprintf("use DROP TABLE to delete "\
> +						 "table %s", space_name);
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

Why not ER_DROP_SPACE?

> +		parse_context->is_aborted = true;
> 		goto exit_drop_table;
> 	}
> 	if (!is_view && space->def->opts.is_view) {
> -		sqlErrorMsg(parse_context, "use DROP VIEW to delete view %s",
> -				space_name);
> +		const char *err_msg = tt_sprintf("use DROP VIEW to delete "\
> +						 "view %s", space_name);
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

The same question.

> +		parse_context->is_aborted = true;
> 		goto exit_drop_table;
> 	}
> 	/*
> @@ -1760,8 +1785,10 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
> 		}
> 	} else {
> 		if (parent_space->def->opts.is_view) {
> -			sqlErrorMsg(parse_context,
> -					"referenced table can't be view");
> +			const char *err_msg = tt_sprintf("referenced table "\
> +							 "can't be view");
> +			diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

ER_CREATE_FK_CONSTRAINT

> +			parse_context->is_aborted = true;
> 			goto exit_create_fk;
> 		}
> 	}
> @@ -2149,7 +2176,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 	struct space_def *def = space->def;
> 
> 	if (def->opts.is_view) {
> -		sqlErrorMsg(parse, "views can not be indexed");
> +		const char *err_msg = tt_sprintf("views can not be indexed");
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);

ER_CREATE_INDEX

> +		parse->is_aborted = true;
> 		goto exit_create_index;
> 	}
> 	/*
> @@ -2947,11 +2976,8 @@ sqlSavepoint(Parse * pParse, int op, Token * pName)
> 			return;
> 		}
> 		if (op == SAVEPOINT_BEGIN &&
> -			sqlCheckIdentifierName(pParse, zName)
> -				!= SQL_OK) {
> -			sqlErrorMsg(pParse, "bad savepoint name");
> +			sqlCheckIdentifierName(pParse, zName) != SQL_OK)

!= 0

> 			return;
> -		}
> 		sqlVdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, P4_DYNAMIC);
> 	}
> }
> @@ -1757,8 +1759,10 @@ sqlExprListAppendVector(Parse * pParse,	/* Parsing context */
> 	 */
> 	if (pExpr->op != TK_SELECT
> 	    && pColumns->nId != (n = sqlExprVectorSize(pExpr))) {
> -		sqlErrorMsg(pParse, "%d columns assigned %d values",
> -				pColumns->nId, n);
> +		const char *err_msg = tt_sprintf("%d columns assigned %d "\
> +						 "values", pColumns->nId, n);
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +		pParse->is_aborted = true;
> 		goto vector_append_error;
> 	}
> 
> @@ -1878,7 +1882,10 @@ sqlExprListCheckLength(Parse * pParse,
> 	testcase(pEList && pEList->nExpr == mx);
> 	testcase(pEList && pEList->nExpr == mx + 1);
> 	if (pEList && pEList->nExpr > mx) {
> -		sqlErrorMsg(pParse, "too many columns in %s", zObject);
> +		const char *err_msg = tt_sprintf("too many columns in %s",
> +						 zObject);

You introduced LIMIT and “too many columns” errors in previous patch.
Why you can’t use them here?

> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +		pParse->is_aborted = true;
> 	}
> }
> 
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index b69f059..3653824 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -897,7 +897,9 @@ expr(A) ::= VARIABLE(X).     {
>   Token t = X;
>   if (pParse->parse_only) {
>     spanSet(&A, &t, &t);
> -    sqlErrorMsg(pParse, "bindings are not allowed in DDL");
> +    const char *err_msg = tt_sprintf("bindings are not allowed in DDL”);

Em, why can’t you inline this var?

diag_set(ClientError, ER_SQL_PARSER_GENERIC, "bindings are not allowed in DDL");

> +    diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +    pParse->is_aborted = true;
>     A.pExpr = NULL;
>   } else if (!(X.z[0]=='#' && sqlIsdigit(X.z[1]))) {
>     u32 n = X.n;
> 
> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 02eca37..63a1b96 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -392,14 +392,25 @@ lookupName(Parse * pParse,	/* The parsing context */
> 					pOrig = pEList->a[j].pExpr;
> 					if ((pNC->ncFlags & NC_AllowAgg) == 0
> 					    && ExprHasProperty(pOrig, EP_Agg)) {
> -						sqlErrorMsg(pParse,
> -								"misuse of aliased aggregate %s",
> -								zAs);
> +						const char *err_msg =
> +							tt_sprintf("misuse of "\
> +								   "aliased "\
> +								   "aggregate "\
> +								   "%s", zAs);

Such formatting looks terrible. Lets break 80 border or move
declaration of this error msg at few blocks above.

> @@ -633,9 +647,13 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
> 						    exprProbability(pList->a[1].
> 								    pExpr);
> 						if (pExpr->iTable < 0) {
> -							sqlErrorMsg(pParse,
> -									"second argument to likelihood() must be a "
> -									"constant between 0.0 and 1.0");
> +							const char *err_msg =
> +								tt_sprintf("second argument to likelihood() must be a "\
> +									   "constant between 0.0 and 1.0”);

You don’t need sprintf here.

> +							diag_set(ClientError,
> +								 ER_SQL_PARSER_GENERIC,
> +								 err_msg);
> +							pParse->is_aborted = true;
> 							pNC->nErr++;
> 						}
> 					} else {
> @@ -802,7 +827,11 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
> 				testcase(pExpr->op == TK_GT);
> 				testcase(pExpr->op == TK_GE);
> 				testcase(pExpr->op == TK_BETWEEN);
> -				sqlErrorMsg(pParse, "row value misused");
> +				const char *err_msg = tt_sprintf("row value "\
> +								 "misused”);

The same.

> +				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +					 err_msg);
> +				pParse->is_aborted = true;
> 			}
> 			break;
> 		}
> @@ -950,7 +964,10 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
> 	db = pParse->db;
> #if SQL_MAX_COLUMN
> 	if (pOrderBy->nExpr > db->aLimit[SQL_LIMIT_COLUMN]) {
> -		sqlErrorMsg(pParse, "too many terms in ORDER BY clause");
> +		const char *err_msg = tt_sprintf("too many terms in ORDER BY "\
> +						 "clause”);

The same.

> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +		pParse->is_aborted = true;
> 		return 1;
> 	}
> #endif
> @@ -1024,9 +1045,11 @@ resolveCompoundOrderBy(Parse * pParse,	/* Parsing context.  Leave error messages
> 	}
> 	for (i = 0; i < pOrderBy->nExpr; i++) {
> 		if (pOrderBy->a[i].done == 0) {
> -			sqlErrorMsg(pParse,
> -					"%r ORDER BY term does not match any "
> -					"column in the result set", i + 1);
> +			const char *err_msg =
> +				tt_sprintf("ORDER BY term does not match any "\
> +					   "column in the result set”);

The same.

> +			diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +			pParse->is_aborted = true;
> 			return 1;
> 		}
> 	}
> @@ -1417,9 +1450,15 @@ resolveSelectStep(Walker * pWalker, Select * p)
> 			for (i = 0, pItem = pGroupBy->a; i < pGroupBy->nExpr;
> 			     i++, pItem++) {
> 				if (ExprHasProperty(pItem->pExpr, EP_Agg)) {
> -					sqlErrorMsg(pParse,
> -							"aggregate functions are not allowed in "
> -							"the GROUP BY clause");
> +					const char *err_msg =
> +						tt_sprintf("aggregate "\
> +							   "functions are not "\
> +							   "allowed in the "\
> +							   "GROUP BY clause”);

Ok, verify all usages of sprintf and make sure they are really required.
I see several more in code.

> @@ -613,8 +622,11 @@ sqlProcessJoin(Parse * pParse, Select * p)
> 		/* Disallow both ON and USING clauses in the same join
> 		 */
> 		if (pRight->pOn && pRight->pUsing) {
> -			sqlErrorMsg(pParse, "cannot have both ON and USING "
> -					"clauses in the same join");
> +			const char *err_msg =
> +				tt_sprintf("cannot have both ON and USING "
> +					   "clauses in the same join");
> +			diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg);
> +			pParse->is_aborted = true;
> 			return 1;
> 		}
> 
> @@ -650,10 +662,16 @@ sqlProcessJoin(Parse * pParse, Select * p)
> 				    || !tableAndColumnIndex(pSrc, i + 1, zName,
> 							    &iLeft, &iLeftCol)
> 				    ) {
> -					sqlErrorMsg(pParse,
> -							"cannot join using column %s - column "
> -							"not present in both tables",
> -							zName);
> +					const char *err_msg =
> +						tt_sprintf("cannot join using "\
> +							   "column %s - "\
> +							   "column not "\
> +							   "present in both "\
> +							   "tables", zName);

Horrible formatting.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 85718e1..5a74c65 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3182,7 +3182,6 @@ void sqlTreeViewWith(TreeView *, const With *);
> #endif
> 
> void sqlSetString(char **, sql *, const char *);
> -void sqlErrorMsg(Parse *, const char *, ...);
> void sqlDequote(char *);
> void sqlNormalizeName(char *z);
> void sqlTokenInit(Token *, char *);
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 2f1268a..297ba01 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -621,8 +621,11 @@ codeTriggerProgram(Parse * pParse,	/* The parser context */
> 	sqlSubProgramsRemaining--;
> 
> 	if (sqlSubProgramsRemaining == 0) {
> -		sqlErrorMsg(pParse,
> -				"Maximum number of chained trigger activations exceeded.");
> +		const char *err_msg = tt_sprintf("Maximum number of chained "\
> +						 "trigger activations "\
> +						 "exceeded.”);

Please, make sure that it is valid error. And the rest of errors
I pointed in google doc file. If this is unreachable code, then
replace error with assert.

> -/*
>  * Convert an SQL-style quoted string into a normal string by removing
>  * the quote characters.  The conversion is done in-place.  If the
>  * input does not begin with a quote character, then this routine
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 9957f3a..d4fbf17 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1222,7 +1222,9 @@ valueFromFunction(sql * db,	/* The database connection */
> 	pFunc->xSFunc(&ctx, nVal, apVal);
> 	if (ctx.isError) {
> 		rc = ctx.isError;
> -		sqlErrorMsg(pCtx->pParse, "%s", sql_value_text(pVal));
> +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +			 sql_value_text(pVal));

What kind of error is it? Please, add reasonable error message,
not only text representation of value.

> +		pCtx->pParse->is_aborted = true;
> 	} else {
> 		sql_value_apply_type(pVal, type);
> 		assert(rc == SQL_OK);
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 33885d0..e2c91e0 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -3914,7 +3914,9 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
> 	}
> 
> 	if (nFrom == 0) {
> -		sqlErrorMsg(pParse, "no query solution");
> +		const char *err_msg = tt_sprintf("no query solution”);

Same: this path seems to be unreachable.

> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index e190ad7..16a31a8 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -1855,13 +1855,13 @@ test:do_catchsql_test(
>     "e_select-8.7.1.1",
>     "SELECT x FROM d1 UNION ALL SELECT a FROM d2 ORDER BY x*z",
>     {
> -        1, "1st ORDER BY term does not match any column in the result set"})
> +        1, "ORDER BY term does not match any column in the result set”}

Why did you change error message?
I see quite a lot affected tests.