[tarantool-patches] Re: [PATCH v4 2/8] sql: set SQL parser errors via diag_set()

[n.pettik]

> On 13 Mar 2019, at 20:03, imeevma at tarantool.org wrote:
> 
> Hi! Thank you for review! Diff between versions and new version of
> patch below.
> 
> Diff between patches:
> 
> commit 61bc67e61298129d66a436d58957bb411b6c9b81
> Author: Mergen Imeev <imeevma at gmail.com>
> Date:   Wed Mar 6 21:27:51 2019 +0300
> 
>    Temporary: Review fix
> 
> diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c
> index e0d2ec8..8812298 100644
> --- a/src/box/sql/malloc.c
> +++ b/src/box/sql/malloc.c
> @@ -55,9 +55,7 @@ sql_sized_malloc(int nByte)
>    p[0] = nByte;
>    p++;
>  } else {
> -   testcase(sqlGlobalConfig.xLog != 0);
> -   sql_log(SQL_NOMEM,
> -         "failed to allocate %u bytes of memory", nByte);
> +   diag_set(OutOfMemory, nByte, "realloc", "p”);

This function doesn’t set mallocFailed flag. A lot of callers
of this function don’d check its return value. So I guess this
could result in installed diag error, but it would be ignored.
Can we set here at least mallocFailed?

>  }
>  return (void *)p;
> }
> @@ -115,10 +113,7 @@ sql_sized_realloc(void *pPrior, int nByte)
>    p[0] = nByte;
>    p++;
>  } else {
> -   testcase(sqlGlobalConfig.xLog != 0);
> -   sql_log(SQL_NOMEM,
> -         "failed memory resize %u to %u bytes",
> -         sql_sized_sizeof(pPrior), nByte);
> +   diag_set(OutOfMemory, nByte, "malloc", "p”);

The same is here.

>  }
>  return (void *)p;
> }
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 0c6296d..828a1ae 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -102,9 +102,8 @@ sqlPrepare(sql * db,  /* Database handle. */
> 
>  if (sParse.rc == SQL_DONE)
>    sParse.rc = SQL_OK;
> - if (db->mallocFailed) {
> -   sParse.rc = SQL_NOMEM;
> - }
> + if (db->mallocFailed)
> +   sParse.rc = SQL_TARANTOOL_ERROR;
>  if (pzTail) {
>    *pzTail = sParse.zTail;
>  }
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 58685c4..834c165 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -483,7 +483,9 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>              &pParse->sLastToken.isReserved);
>      i += pParse->sLastToken.n;
>      if (i > mxSqlLen) {
> -       pParse->rc = SQL_TOOBIG;
> +       diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +          "string or blob too big”);

I would add to error message max possible length.

> +       pParse->rc = SQL_TARANTOOL_ERROR;
>        break;
>      }
>    } else {
> @@ -502,7 +504,9 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>      assert(tokenType == TK_SPACE
>             || tokenType == TK_ILLEGAL);
>      if (db->u1.isInterrupted) {
> -       pParse->rc = SQL_INTERRUPT;
> +       diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> +          "interrupted”);

What does it mean? AFAIR it is dead code (i.e. everything
connected with “interrupt”).

> +       pParse->rc = SQL_TARANTOOL_ERROR;
>        break;
>      }
>      if (tokenType == TK_ILLEGAL) {