[tarantool-patches] Re: [PATCH v3 5/9] sql: remove field zErrMsg of struct Parse

["n.pettik" <korablev@tarantool.org>]

>>> @@ -146,11 +145,7 @@ sqlPrepare(sql * db,  /* Database handle. */
>>>   *ppStmt = (sql_stmt *) sParse.pVdbe;
>>> }
>>> 
>>> -  if (zErrMsg) {
>>> -    sqlErrorWithMsg(db, rc, "%s", zErrMsg);
>>> -  } else {
>>> -    sqlError(db, rc);
>>> -  }
>>> +  sqlError(db, rc);
>> 
>> sqlError seems to be useless/dead. Please, make a note somewhere
>> to remove it as follow-up to error-refactoring patch-set.
>> 
> Ok, made a note.

And where is it? Please, paste a link to it.

> New version:
> 
> commit 29c77ea3f8463994b98bcb23653f901cf46e472a
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Wed Feb 27 09:40:17 2019 +0300
> 
>    sql: remove field zErrMsg of struct Parse
> 
>    This field become unused and should be removed.
> 
>    Part of #3965
> 
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 42737ff..4359fa7 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -52,7 +52,6 @@ sqlPrepare(sql * db,  /* Database handle. */
>         const char **pzTail  /* OUT: End of parsed string */
>     )
> {
> - char *zErrMsg = 0;  /* Error message */
>  int rc = SQL_OK;  /* Result code */
>  Parse sParse;   /* Parsing context */
>  sql_parser_create(&sParse, db);
> @@ -89,14 +88,14 @@ sqlPrepare(sql * db,  /* Database handle. */
>    }
>    zSqlCopy = sqlDbStrNDup(db, zSql, nBytes);
>    if (zSqlCopy) {
> -     sqlRunParser(&sParse, zSqlCopy, &zErrMsg);
> +     sqlRunParser(&sParse, zSqlCopy);
>      sParse.zTail = &zSql[sParse.zTail - zSqlCopy];
>      sqlDbFree(db, zSqlCopy);
>    } else {
>      sParse.zTail = &zSql[nBytes];
>    }
>  } else {
> -   sqlRunParser(&sParse, zSql, &zErrMsg);
> +   sqlRunParser(&sParse, zSql);
>  }
>  assert(0 == sParse.nQueryLoop);
> 
> @@ -169,11 +168,7 @@ sqlPrepare(sql * db, /* Database handle. */
>    *ppStmt = (sql_stmt *) sParse.pVdbe;
>  }
> 
> - if (zErrMsg) {
> -   sqlErrorWithMsg(db, rc, "%s", zErrMsg);
> - } else {
> -   sqlError(db, rc);
> - }
> + sqlError(db, rc);

As we pointed out, you can remove this call.

> 
>  /* Delete any TriggerPrg structures allocated while parsing this statement. */
>  while (sParse.pTriggerPrg) {
> @@ -318,7 +313,6 @@ sql_parser_destroy(Parse *parser)
>    db->lookaside.bDisable -= parser->disableLookaside;
>  }
>  parser->disableLookaside = 0;
> - sqlDbFree(db, parser->zErrMsg);
>  switch (parser->parsed_ast_type) {
>  case AST_TYPE_SELECT:
>    sql_select_delete(db, parser->parsed_ast.select);
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index cfac553..66cbc73 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -5431,9 +5431,7 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
>  * The results are returned according to the SelectDest structure.
>  * See comments in sqlInt.h for further information.
>  *
> - * This routine returns the number of errors.  If any errors are
> - * encountered, then an appropriate error message is left in
> - * pParse->zErrMsg.
> + * This routine returns the number of errors.

That’s not true:

        rc = pParse->is_aborted;

       /* Control jumps to here if an error is encountered above, or upon
        * successful coding of the SELECT.
        */
 select_end:
       pParse->iSelectId = iRestoreSelectId;

       /* Identify column names if results of the SELECT are to be output.
        */
       if (rc == SQL_OK && pDest->eDest == SRT_Output) {
              generateColumnNames(pParse, pTabList, pEList);
       }

       sqlDbFree(db, sAggInfo.aCol);
       sqlDbFree(db, sAggInfo.aFunc);
#ifdef SQL_DEBUG
       SELECTTRACE(1, pParse, p, ("end processing\n"));
       pParse->nSelectIndent--;
#endif
return rc;

So it doesn’t return count of occurred errors.

> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 4ca3c2e..1a7248e 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -439,13 +439,10 @@ parser_space_delete(struct sql *db, struct space *space)
> 
> /*
>  * Run the parser on the given SQL string.  The parser structure is
> - * passed in.  An SQL_ status code is returned.  If an error occurs
> - * then an and attempt is made to write an error message into
> - * memory obtained from sql_malloc() and to make *pzErrMsg point to that
> - * error message.
> + * passed in.  An SQL_ status code is returned.

On the other hand:

return nErr;

So what is the truth?

>  */
> int
> -sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
> +sqlRunParser(Parse * pParse, const char *zSql)
> {
>  int nErr = 0;   /* Number of errors encountered */
>  int i;      /* Loop counter */