[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