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 C6A7428C24 for ; Tue, 5 Mar 2019 07:16:17 -0500 (EST) 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 qMDLCHgQPrOv for ; Tue, 5 Mar 2019 07:16:17 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5FF6528AA3 for ; Tue, 5 Mar 2019 07:16:15 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v3 9/9] sql: remove sqlErrorMsg() From: "n.pettik" In-Reply-To: Date: Tue, 5 Mar 2019 15:16:12 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <35851381-BED4-4B82-923B-20D0984504E1@tarantool.org> References: 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: Imeev Mergen > @@ -1284,10 +1302,13 @@ sql_create_view(struct Parse *parse_context, = struct Token *begin, > goto create_view_fail; > if (aliases !=3D NULL) { > if ((int)select_res_space->def->field_count !=3D = 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 =3D > + 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D true; > goto exit_create_fk; > } > } > @@ -2149,7 +2176,9 @@ sql_create_index(struct Parse *parse, struct = Token *token, > struct space_def *def =3D space->def; >=20 > if (def->opts.is_view) { > - sqlErrorMsg(parse, "views can not be indexed"); > + const char *err_msg =3D tt_sprintf("views can not be = indexed"); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg); ER_CREATE_INDEX > + parse->is_aborted =3D true; > goto exit_create_index; > } > /* > @@ -2947,11 +2976,8 @@ sqlSavepoint(Parse * pParse, int op, Token * = pName) > return; > } > if (op =3D=3D SAVEPOINT_BEGIN && > - sqlCheckIdentifierName(pParse, zName) > - !=3D SQL_OK) { > - sqlErrorMsg(pParse, "bad savepoint name"); > + sqlCheckIdentifierName(pParse, zName) !=3D = SQL_OK) !=3D 0 > return; > - } > sqlVdbeAddOp4(v, OP_Savepoint, op, 0, 0, zName, = P4_DYNAMIC); > } > } > @@ -1757,8 +1759,10 @@ sqlExprListAppendVector(Parse * pParse, = /* Parsing context */ > */ > if (pExpr->op !=3D TK_SELECT > && pColumns->nId !=3D (n =3D sqlExprVectorSize(pExpr))) { > - sqlErrorMsg(pParse, "%d columns assigned %d values", > - pColumns->nId, n); > + const char *err_msg =3D tt_sprintf("%d columns assigned = %d "\ > + "values", = pColumns->nId, n); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg); > + pParse->is_aborted =3D true; > goto vector_append_error; > } >=20 > @@ -1878,7 +1882,10 @@ sqlExprListCheckLength(Parse * pParse, > testcase(pEList && pEList->nExpr =3D=3D mx); > testcase(pEList && pEList->nExpr =3D=3D mx + 1); > if (pEList && pEList->nExpr > mx) { > - sqlErrorMsg(pParse, "too many columns in %s", zObject); > + const char *err_msg =3D tt_sprintf("too many columns in = %s", > + zObject); You introduced LIMIT and =E2=80=9Ctoo many columns=E2=80=9D errors in = previous patch. Why you can=E2=80=99t use them here? > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg); > + pParse->is_aborted =3D true; > } > } >=20 > 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) ::=3D VARIABLE(X). { > Token t =3D X; > if (pParse->parse_only) { > spanSet(&A, &t, &t); > - sqlErrorMsg(pParse, "bindings are not allowed in DDL"); > + const char *err_msg =3D tt_sprintf("bindings are not allowed in = DDL=E2=80=9D); Em, why can=E2=80=99t 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 =3D true; > A.pExpr =3D NULL; > } else if (!(X.z[0]=3D=3D'#' && sqlIsdigit(X.z[1]))) { > u32 n =3D X.n; >=20 > 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 =3D pEList->a[j].pExpr; > if ((pNC->ncFlags & NC_AllowAgg) = =3D=3D 0 > && ExprHasProperty(pOrig, = EP_Agg)) { > - sqlErrorMsg(pParse, > - "misuse = of aliased aggregate %s", > - zAs); > + const char *err_msg =3D > + = 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 =3D > + = tt_sprintf("second argument to likelihood() must be a "\ > + = "constant between 0.0 and 1.0=E2=80=9D); You don=E2=80=99t need sprintf here. > + = diag_set(ClientError, > + = ER_SQL_PARSER_GENERIC, > + = err_msg); > + = pParse->is_aborted =3D true; > pNC->nErr++; > } > } else { > @@ -802,7 +827,11 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > testcase(pExpr->op =3D=3D TK_GT); > testcase(pExpr->op =3D=3D TK_GE); > testcase(pExpr->op =3D=3D TK_BETWEEN); > - sqlErrorMsg(pParse, "row value = misused"); > + const char *err_msg =3D tt_sprintf("row = value "\ > + = "misused=E2=80=9D); The same. > + diag_set(ClientError, = ER_SQL_PARSER_GENERIC, > + err_msg); > + pParse->is_aborted =3D true; > } > break; > } > @@ -950,7 +964,10 @@ resolveCompoundOrderBy(Parse * pParse, /* = Parsing context. Leave error messages > db =3D 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 =3D tt_sprintf("too many terms in = ORDER BY "\ > + "clause=E2=80=9D); The same. > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err_msg); > + pParse->is_aborted =3D true; > return 1; > } > #endif > @@ -1024,9 +1045,11 @@ resolveCompoundOrderBy(Parse * pParse, /* = Parsing context. Leave error messages > } > for (i =3D 0; i < pOrderBy->nExpr; i++) { > if (pOrderBy->a[i].done =3D=3D 0) { > - sqlErrorMsg(pParse, > - "%r ORDER BY term does not match = any " > - "column in the result set", i + = 1); > + const char *err_msg =3D > + tt_sprintf("ORDER BY term does not match = any "\ > + "column in the result = set=E2=80=9D); The same. > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, = err_msg); > + pParse->is_aborted =3D true; > return 1; > } > } > @@ -1417,9 +1450,15 @@ resolveSelectStep(Walker * pWalker, Select * p) > for (i =3D 0, pItem =3D 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 =3D > + tt_sprintf("aggregate "\ > + "functions = are not "\ > + "allowed in = the "\ > + "GROUP BY = clause=E2=80=9D); 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 =3D > + 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 =3D true; > return 1; > } >=20 > @@ -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 =3D > + 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 >=20 > 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--; >=20 > if (sqlSubProgramsRemaining =3D=3D 0) { > - sqlErrorMsg(pParse, > - "Maximum number of chained trigger = activations exceeded."); > + const char *err_msg =3D tt_sprintf("Maximum number of = chained "\ > + "trigger activations "\ > + "exceeded.=E2=80=9D); 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 =3D 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 =3D true; > } else { > sql_value_apply_type(pVal, type); > assert(rc =3D=3D 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) > } >=20 > if (n=46rom =3D=3D 0) { > - sqlErrorMsg(pParse, "no query solution"); > + const char *err_msg =3D tt_sprintf("no query = solution=E2=80=9D); 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=E2=80=9D} Why did you change error message? I see quite a lot affected tests.