From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v4 8/8] sql: remove sqlErrorMsg() Date: Fri, 15 Mar 2019 16:36:46 +0300 [thread overview] Message-ID: <0D9B8996-D469-498B-8D6C-AFD90C6DD35E@tarantool.org> (raw) In-Reply-To: <99f4adc61352899a83a7097b965845bafaa968ef.1552494059.git.imeevma@gmail.com> >>> + 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 > 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 >>> >>> 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. > 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 = 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. > I did, but not sure if this error is reachable. I wasn't able to > reproduce it. See comment above. >>> + 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. > I think so too, but left as it is for now. The same. > New version: > > commit 99f4adc61352899a83a7097b965845bafaa968ef > Author: Mergen Imeev <imeevma@gmail.com> > Date: Sat Mar 9 18:54:19 2019 +0300 > > sql: remove sqlErrorMsg() > > This patch completely replaces sqlErrorMsg() with diag_set() and > removes sqlErrorMsg(). > > Closes #3965 > Closes #3036 I wouldn’t close both issues since there’s 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 = sqlVdbeMakeLabel(v); > > if (nLeft != sqlExprVectorSize(pRight)) { > - sqlErrorMsg(pParse, "row value misused"); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "row value misused”); I see a lot of “row value misused” errors in source code, but only one test case on this error. > @@ -2959,15 +2935,26 @@ int > sqlExprCheckIN(Parse * pParse, Expr * pIn) > { > int nVector = sqlExprVectorSize(pIn->pLeft); > + const char *err; > if ((pIn->flags & EP_xIsSelect)) { > if (nVector != pIn->x.pSelect->pEList->nExpr) { > - sqlSubselectError(pParse, > - pIn->x.pSelect->pEList->nExpr, > - nVector); > + err = "sub-select returns %d columns - expected %d"; > + int expr_count = pIn->x.pSelect->pEList->nExpr; > + err = tt_sprintf(err, expr_count, nVector); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pParse->is_aborted = true; > return 1; > } > } else if (nVector != 1) { > - sqlVectorErrorMsg(pParse, pIn->pLeft); > + if (pIn->pLeft->flags & EP_xIsSelect) { > + err = "sub-select returns %d columns - expected 1"; > + int expr_count = pIn->pLeft->x.pSelect->pEList->nExpr; > + err = tt_sprintf(err, expr_count); > + } else { > + err = "row value misused”; 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 != pIn->x.pSelect->pEList->nExpr) { err = "sub-select returns %d columns - expected %d"; int expr_count = pIn->x.pSelect->pEList->nExpr; - err = 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 = true; return 1; } } else if (nVector != 1) { if (pIn->pLeft->flags & EP_xIsSelect) { - err = "sub-select returns %d columns - expected 1"; int expr_count = pIn->pLeft->x.pSelect->pEList->nExpr; - err = tt_sprintf(err, expr_count); + err = tt_sprintf("sub-select returns %d columns - "\ + "expected 1", expr_count); > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pParse->is_aborted = true; > return 1; > } > return 0; > @@ -4134,7 +4122,12 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > testcase(op == TK_SELECT); > if (op == TK_SELECT > && (nCol = pExpr->x.pSelect->pEList->nExpr) != 1) { > - sqlSubselectError(pParse, nCol, 1); > + const char *err = "sub-select returns %d "\ > + "columns - expected 1"; > + err = tt_sprintf(err, nCol); @@ -4124,9 +4124,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) && (nCol = pExpr->x.pSelect->pEList->nExpr) != 1) { const char *err = "sub-select returns %d "\ "columns - expected 1"; - err = 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 */ > > if (pColumn != 0 && nColumn != pColumn->nId) { > - sqlErrorMsg(pParse, "%d values for %d columns", nColumn, > - pColumn->nId); > + const char *err = "%d values for %d columns"; > + err = 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 != 0 && nColumn != pColumn->nId) { const char *err = "%d values for %d columns"; - err = 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 = true; goto insert_cleanup; > + pParse->is_aborted = true; > goto insert_cleanup; > } > > > // 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 == 0); > assert(pExpr->x.pSelect == 0); > pOrig = pEList->a[j].pExpr; > + const char *err = "misuse of aliased "\ > + "aggregate %s"; > if ((pNC->ncFlags & NC_AllowAgg) == 0 > && ExprHasProperty(pOrig, EP_Agg)) { > - sqlErrorMsg(pParse, > - "misuse of aliased aggregate %s", > - zAs); > + err = 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) == 0 && ExprHasProperty(pOrig, EP_Agg)) { - err = tt_sprintf(err, zAs); diag_set(ClientError, ER_SQL_PARSER_GENERIC, - err); + tt_sprintf(err, zAs)); pParse->is_aborted = true; > @@ -697,9 +708,12 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > pParse->is_aborted = true; > pNC->nErr++; > } else if (wrong_num_args) { > - sqlErrorMsg(pParse, > - "wrong number of arguments to function %.*s()", > - nId, zId); > + const char *err = "wrong number of arguments "\ > + "to function %.*s()"; > + err = 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 = "wrong number of arguments "\ "to function %.*s()"; - err = tt_sprintf(err, nId, zId); diag_set(ClientError, ER_SQL_PARSER_GENERIC, - err); + tt_sprintf(err, nId, zId)); pParse->is_aborted = true; pNC->nErr++; > @@ -980,9 +981,16 @@ resolveCompoundOrderBy(Parse * pParse, /* Parsing context. Leave error messages > pE = sqlExprSkipCollate(pItem->pExpr); > if (sqlExprIsInteger(pE, &iCol)) { > if (iCol <= 0 || iCol > pEList->nExpr) { > - resolveOutOfRangeError(pParse, "ORDER", > - i + 1, > - pEList->nExpr); > + const char *err = > + "Error at ORDER BY in place "\ > + "%d: term out of range - "\ > + "should be between 1 and %d"; > + err = 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 = tt_sprintf(err, i + 1, pEList->nExpr); diag_set(ClientError, - ER_SQL_PARSER_GENERIC, - err); + ER_SQL_PARSER_GENERIC, err); pParse->is_aborted = true; return 1; > @@ -1028,9 +1036,12 @@ 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 = "Error at ORDER BY in place %d: "\ > + "term does not match any column in "\ > + "the result set"; > + err = 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 = "Error at ORDER BY in place %d: "\ "term does not match any column in "\ "the result set"; - err = 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 != 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 = > + "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 = tt_sprintf(err_msg, > + "UNION ALL"); > + break; Why don’t use selectOpName? Like it was in sqlSelectWrongNumTermsError(). > + case TK_INTERSECT: > + err_msg = tt_sprintf(err_msg, > + "INTERSECT"); > + break; > + case TK_EXCEPT: > + err_msg = tt_sprintf(err_msg, "EXCEPT"); > + break; > + default: > + err_msg = tt_sprintf(err_msg, "UNION"); > + break; > + } > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + err_msg); > + } > + pParse->is_aborted = true; > return WRC_Abort; > > 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 = ctx.isError; > - sqlErrorMsg(pCtx->pParse, "%s", sql_value_text(pVal)); > + const char *err = "Error in function '%s': %s”; It doesn’t look better than previous variant. If you don’t know what kind of error this is and how it can appear - just replace it with assertion. > + err = tt_sprintf(err, pFunc->zName, sql_value_text(pVal)); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, err); > + pCtx->pParse->is_aborted = true; > } else { > sql_value_apply_type(pVal, type); > assert(rc == SQL_OK); > } > - if (rc != SQL_OK) > - pCtx->pParse->is_aborted = true; > > value_from_function_out: > if (rc != 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) > } > > if (nFrom == 0) { > - sqlErrorMsg(pParse, "no query solution"); > + diag_set(ClientError, ER_SQL_PARSER_GENERIC, > + "no query solution”); As I already said in previous review - replace with assertion and comment. > + pParse->is_aborted = true; > sqlDbFree(db, pSpace); > return SQL_ERROR; > }
next prev parent reply other threads:[~2019-03-15 13:36 UTC|newest] Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-03-13 17:03 [tarantool-patches] [PATCH v4 0/8] sql: use diag_set() for errors in SQL imeevma 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 1/8] sql: rework syntax errors imeevma 2019-03-14 18:24 ` [tarantool-patches] " n.pettik 2019-03-14 18:28 ` Imeev Mergen 2019-03-15 14:09 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 2/8] sql: set SQL parser errors via diag_set() imeevma 2019-03-14 19:26 ` [tarantool-patches] " n.pettik 2019-03-14 19:36 ` n.pettik 2019-03-18 15:06 ` Mergen Imeev 2019-03-19 9:41 ` n.pettik 2019-03-19 11:24 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse imeevma 2019-03-14 19:53 ` [tarantool-patches] " n.pettik 2019-03-18 15:28 ` Mergen Imeev 2019-03-19 9:54 ` n.pettik 2019-03-19 13:17 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 4/8] sql: remove field nErr from " imeevma 2019-03-14 19:58 ` [tarantool-patches] " n.pettik 2019-03-19 13:27 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 5/8] sql: remove field zErrMsg " imeevma 2019-03-14 22:15 ` [tarantool-patches] " n.pettik 2019-03-19 13:20 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 6/8] sql: rework three errors of "unsupported" type imeevma 2019-03-14 22:15 ` [tarantool-patches] " n.pettik 2019-03-19 13:30 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 7/8] sql: rework semantic errors imeevma 2019-03-15 15:49 ` [tarantool-patches] " n.pettik 2019-03-22 12:48 ` Mergen Imeev 2019-03-26 14:14 ` n.pettik 2019-03-26 16:56 ` Mergen Imeev 2019-03-26 18:16 ` n.pettik 2019-03-26 19:20 ` Mergen Imeev 2019-03-26 21:36 ` n.pettik 2019-03-27 6:48 ` Kirill Yukhin 2019-03-13 17:03 ` [tarantool-patches] [PATCH v4 8/8] sql: remove sqlErrorMsg() imeevma 2019-03-15 13:36 ` n.pettik [this message] 2019-03-25 18:47 ` [tarantool-patches] " Mergen Imeev 2019-03-26 13:34 ` n.pettik 2019-03-26 17:52 ` Mergen Imeev 2019-03-26 18:28 ` n.pettik 2019-03-26 19:21 ` Mergen Imeev 2019-03-26 21:36 ` n.pettik 2019-03-27 6:49 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0D9B8996-D469-498B-8D6C-AFD90C6DD35E@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v4 8/8] sql: remove sqlErrorMsg()' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox