[tarantool-patches] Re: [PATCH v4 8/8] sql: remove sqlErrorMsg()
n.pettik
korablev at tarantool.org
Fri Mar 15 16:36:46 MSK 2019
>>> + 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 at 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;
> }
More information about the Tarantool-patches
mailing list