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 1510C295A7 for ; Fri, 15 Mar 2019 09:36:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XAImS4ruxm5m for ; Fri, 15 Mar 2019 09:36:49 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 08B66267A0 for ; Fri, 15 Mar 2019 09:36:48 -0400 (EDT) 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 v4 8/8] sql: remove sqlErrorMsg() From: "n.pettik" In-Reply-To: <99f4adc61352899a83a7097b965845bafaa968ef.1552494059.git.imeevma@gmail.com> Date: Fri, 15 Mar 2019 16:36:46 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0D9B8996-D469-498B-8D6C-AFD90C6DD35E@tarantool.org> References: <99f4adc61352899a83a7097b965845bafaa968ef.1552494059.git.imeevma@gmail.com> 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 >>> + 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) >>=20 >> !=3D 0 > Function sqlCheckIdentifierName() still works in terms of > {SQL_OK, SQL_ERROR}, so I left this part as it was. Under the hood SQL_OK is 0. We are going to remove this status codes, so let's fix it now. >>> 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); >>=20 >> 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. > This error is valid. As for the other ones - I couldn't reach some > of them, but didn't remove them for now. Why? Comment code and place assertion instead. Otherwise, provide solid argument to avoid doing that. >>> -/* >>> * 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)); >>=20 >> What kind of error is it? Please, add reasonable error message, >> not only text representation of value. > I did, but not sure if this error is reachable. I wasn't able to > reproduce it. See comment above. >>> + 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); >>=20 >> Same: this path seems to be unreachable. > I think so too, but left as it is for now. The same. > New version: >=20 > commit 99f4adc61352899a83a7097b965845bafaa968ef > Author: Mergen Imeev > Date: Sat Mar 9 18:54:19 2019 +0300 >=20 > sql: remove sqlErrorMsg() >=20 > This patch completely replaces sqlErrorMsg() with diag_set() and > removes sqlErrorMsg(). >=20 > Closes #3965 > Closes #3036 I wouldn=E2=80=99t close both issues since there=E2=80=99s still place = to work on: split generic error into specific ones, use diag for vdbe. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index de06ee0..7205cb0 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -668,7 +668,9 @@ codeVectorCompare(Parse * pParse, /* Code = generator context */ > int addrDone =3D sqlVdbeMakeLabel(v); >=20 > if (nLeft !=3D sqlExprVectorSize(pRight)) { > - sqlErrorMsg(pParse, "row value misused"); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "row value misused=E2=80=9D); I see a lot of =E2=80=9Crow value misused=E2=80=9D errors in source = code, but only one test case on this error. > @@ -2959,15 +2935,26 @@ int > sqlExprCheckIN(Parse * pParse, Expr * pIn) > { > int nVector =3D sqlExprVectorSize(pIn->pLeft); > + const char *err; > if ((pIn->flags & EP_xIsSelect)) { > if (nVector !=3D pIn->x.pSelect->pEList->nExpr) { > - sqlSubselectError(pParse, > - pIn->x.pSelect->pEList->nExpr, > - nVector); > + err =3D "sub-select returns %d columns - expected %d"; > + int expr_count =3D pIn->x.pSelect->pEList->nExpr; > + err =3D tt_sprintf(err, expr_count, nVector); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pParse->is_aborted =3D true; > return 1; > } > } else if (nVector !=3D 1) { > - sqlVectorErrorMsg(pParse, pIn->pLeft); > + if (pIn->pLeft->flags & EP_xIsSelect) { > + err =3D "sub-select returns %d columns - expected 1"; > + int expr_count =3D pIn->pLeft->x.pSelect->pEList->nExpr; > + err =3D tt_sprintf(err, expr_count); > + } else { > + err =3D "row value misused=E2=80=9D; A bit shorter version. Please apply this and other diffs: diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 7205cb0a8..344e1ab9a 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2940,16 +2940,16 @@ sqlExprCheckIN(Parse * pParse, Expr * pIn) if (nVector !=3D pIn->x.pSelect->pEList->nExpr) { err =3D "sub-select returns %d columns - = expected %d"; int expr_count =3D = pIn->x.pSelect->pEList->nExpr; - err =3D tt_sprintf(err, expr_count, nVector); - diag_set(ClientError, ER_SQL_PARSER_GENERIC, = err); + diag_set(ClientError, ER_SQL_PARSER_GENERIC, + tt_sprintf(err, expr_count, nVector);); pParse->is_aborted =3D true; return 1; } } else if (nVector !=3D 1) { if (pIn->pLeft->flags & EP_xIsSelect) { - err =3D "sub-select returns %d columns - = expected 1"; int expr_count =3D = pIn->pLeft->x.pSelect->pEList->nExpr; - err =3D tt_sprintf(err, expr_count); + err =3D tt_sprintf("sub-select returns %d = columns - "\ + "expected 1", expr_count); > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pParse->is_aborted =3D true; > return 1; > } > return 0; > @@ -4134,7 +4122,12 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) > testcase(op =3D=3D TK_SELECT); > if (op =3D=3D TK_SELECT > && (nCol =3D pExpr->x.pSelect->pEList->nExpr) !=3D 1) { > - sqlSubselectError(pParse, nCol, 1); > + const char *err =3D "sub-select returns %d "\ > + "columns - expected 1"; > + err =3D tt_sprintf(err, nCol); @@ -4124,9 +4124,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) && (nCol =3D = pExpr->x.pSelect->pEList->nExpr) !=3D 1) { const char *err =3D "sub-select returns = %d "\ "columns - expected = 1"; - err =3D tt_sprintf(err, nCol); diag_set(ClientError, = ER_SQL_PARSER_GENERIC, - err); + tt_sprintf(err, nCol);); > @@ -510,14 +511,19 @@ sqlInsert(Parse * pParse, /* Parser context */ >=20 > if (pColumn !=3D 0 && nColumn !=3D pColumn->nId) { > - sqlErrorMsg(pParse, "%d values for %d columns", nColumn, > - pColumn->nId); > + const char *err =3D "%d values for %d columns"; > + err =3D tt_sprintf(err, nColumn, pColumn->nId); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 7635f7b69..6f30ab37e 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -521,8 +521,8 @@ sqlInsert(Parse * pParse, /* Parser context */ } if (pColumn !=3D 0 && nColumn !=3D pColumn->nId) { const char *err =3D "%d values for %d columns"; - err =3D tt_sprintf(err, nColumn, pColumn->nId); - diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); + diag_set(ClientError, ER_SQL_PARSER_GENERIC, + tt_sprintf(err, nColumn, pColumn->nId)); pParse->is_aborted =3D true; goto insert_cleanup; > + pParse->is_aborted =3D true; > goto insert_cleanup; > } >=20 >=20 > // Disallow the INDEX BY and NOT INDEXED clauses on UPDATE and DELETE > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index 9171d05..6720914 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -390,16 +390,22 @@ lookupName(Parse * pParse, /* The parsing = context */ > assert(pExpr->x.pList =3D=3D 0); > assert(pExpr->x.pSelect =3D=3D 0); > pOrig =3D pEList->a[j].pExpr; > + const char *err =3D "misuse of aliased "\ > + "aggregate %s"; > if ((pNC->ncFlags & NC_AllowAgg) =3D=3D 0 > && ExprHasProperty(pOrig, EP_Agg)) { > - sqlErrorMsg(pParse, > - "misuse of aliased aggregate %s", > - zAs); > + err =3D tt_sprintf(err, zAs); > + diag_set(ClientError, > + ER_SQL_PARSER_GENERIC, > + err); diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index 672091465..30f2b1f5a 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -394,10 +394,9 @@ lookupName(Parse * pParse, /* The parsing context = */ "aggregate = %s"; if ((pNC->ncFlags & NC_AllowAgg) = =3D=3D 0 && ExprHasProperty(pOrig, = EP_Agg)) { - err =3D tt_sprintf(err, = zAs); diag_set(ClientError, = ER_SQL_PARSER_GENERIC, - err); + tt_sprintf(err, = zAs)); pParse->is_aborted =3D = true; > @@ -697,9 +708,12 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > pParse->is_aborted =3D true; > pNC->nErr++; > } else if (wrong_num_args) { > - sqlErrorMsg(pParse, > - "wrong number of arguments to function %.*s()", > - nId, zId); > + const char *err =3D "wrong number of arguments "\ > + "to function %.*s()"; > + err =3D tt_sprintf(err, nId, zId); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + err); @@ -710,9 +709,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) } else if (wrong_num_args) { const char *err =3D "wrong number of = arguments "\ "to function %.*s()"; - err =3D tt_sprintf(err, nId, zId); diag_set(ClientError, = ER_SQL_PARSER_GENERIC, - err); + tt_sprintf(err, nId, zId)); pParse->is_aborted =3D true; pNC->nErr++; > @@ -980,9 +981,16 @@ resolveCompoundOrderBy(Parse * pParse, /* Parsing = context. Leave error messages > pE =3D sqlExprSkipCollate(pItem->pExpr); > if (sqlExprIsInteger(pE, &iCol)) { > if (iCol <=3D 0 || iCol > pEList->nExpr) { > - resolveOutOfRangeError(pParse, "ORDER", > - i + 1, > - pEList->nExpr); > + const char *err =3D > + "Error at ORDER BY in place "\ > + "%d: term out of range - "\ > + "should be between 1 and %d"; > + err =3D tt_sprintf(err, i + 1, > + pEList->nExpr); > + diag_set(ClientError, > + ER_SQL_PARSER_GENERIC, > + err); @@ -988,8 +986,7 @@ resolveCompoundOrderBy(Parse * pParse, /* = Parsing context. Leave error messages err =3D tt_sprintf(err, i + 1, pEList->nExpr); diag_set(ClientError, - ER_SQL_PARSER_GENERIC, - err); + ER_SQL_PARSER_GENERIC, = err); pParse->is_aborted =3D true; return 1; > @@ -1028,9 +1036,12 @@ 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 =3D "Error at ORDER BY in place %d: "\ > + "term does not match any column in "\ > + "the result set"; > + err =3D tt_sprintf(err, i + 1); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); @@ -1039,8 +1036,8 @@ resolveCompoundOrderBy(Parse * pParse, /* = Parsing context. Leave error messages const char *err =3D "Error at ORDER BY in place = %d: "\ "term does not match any = column in "\ "the result set"; - err =3D tt_sprintf(err, i + 1); - diag_set(ClientError, ER_SQL_PARSER_GENERIC, = err); + diag_set(ClientError, ER_SQL_PARSER_GENERIC, + tt_sprintf(err, i + 1)); > @@ -1444,10 +1469,37 @@ resolveSelectStep(Walker * pWalker, Select * = p) > * number of expressions in the select list. > */ > if (p->pNext && p->pEList->nExpr !=3D p->pNext->pEList->nExpr) { > - sqlSelectWrongNumTermsError(pParse, p->pNext); > + if (p->pNext->selFlags & SF_Values) { > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "all VALUES must have the same "\ > + "number of terms"); > + } else { > + const char *err_msg =3D > + "SELECTs to the left and right of %s "\ > + "do not have the same number of "\ > + "result columns"; > + switch (p->pNext->op) { > + case TK_ALL: > + err_msg =3D tt_sprintf(err_msg, > + "UNION ALL"); > + break; Why don=E2=80=99t use selectOpName? Like it was in sqlSelectWrongNumTermsError(). > + case TK_INTERSECT: > + err_msg =3D tt_sprintf(err_msg, > + "INTERSECT"); > + break; > + case TK_EXCEPT: > + err_msg =3D tt_sprintf(err_msg, "EXCEPT"); > + break; > + default: > + err_msg =3D tt_sprintf(err_msg, "UNION"); > + break; > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + err_msg); > + } > + pParse->is_aborted =3D true; > return WRC_Abort; >=20 > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c > index f417c49..9f0dee4 100644 > --- a/src/box/sql/vdbemem.c > +++ b/src/box/sql/vdbemem.c > @@ -1210,13 +1210,14 @@ 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)); > + const char *err =3D "Error in function '%s': %s=E2=80=9D; It doesn=E2=80=99t look better than previous variant. If you don=E2=80=99t= know what kind of error this is and how it can appear - just replace it with assertion. > + err =3D tt_sprintf(err, pFunc->zName, sql_value_text(pVal)); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pCtx->pParse->is_aborted =3D true; > } else { > sql_value_apply_type(pVal, type); > assert(rc =3D=3D SQL_OK); > } > - if (rc !=3D SQL_OK) > - pCtx->pParse->is_aborted =3D true; >=20 > value_from_function_out: > if (rc !=3D SQL_OK) { > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index 33885d0..1e8c2e0 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"); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "no query solution=E2=80=9D); As I already said in previous review - replace with assertion and comment. > + pParse->is_aborted =3D true; > sqlDbFree(db, pSpace); > return SQL_ERROR; > }