[tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg()
n.pettik
korablev at tarantool.org
Tue Mar 5 15:16:12 MSK 2019
> @@ -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.
More information about the Tarantool-patches
mailing list