[tarantool-patches] Re: [PATCH v3 3/9] sql: remove field nErr of struct Parse
n.pettik
korablev at tarantool.org
Tue Mar 5 12:06:26 MSK 2019
> New version:
>
> commit 0448cc416f3401eeecbad1883fea7193399a72aa
> Author: Mergen Imeev <imeevma at gmail.com>
> Date: Tue Feb 26 22:22:26 2019 +0300
>
> sql: remove field nErr of struct Parse
Nit: remove from (not of).
>
> At the moment, the only purpose of the field nErr of struct Parse
> is to show whether the field rc of the same struct is SQL_OK or
> SQL_TARANTOOL_ERROR. Let's remove it.
>
> Part of #3965
The same problem with ordering here: you replaced nErr > 0 check
with rc == SQL_TARANTOOL_ERROR and then replaced it with
is_aborted. If you firstly replaced rc with is_aborted and then
nErr > 0 -> is_aborted - diff would be slightly smaller.
> @@ -145,9 +144,11 @@ sql_finish_coding(struct Parse *parse_context)
> "Exit with an error if CREATE statement fails"));
> }
>
> - if (db->mallocFailed || parse_context->nErr != 0) {
> - if (parse_context->rc == SQL_OK)
> - parse_context->rc = SQL_ERROR;
> + if (parse_context->rc == SQL_TARANTOOL_ERROR)
> + return;
> + if (db->mallocFailed) {
> + diag_set(OutOfMemory, 0, "SQL", "db”);
It is not a place to set OOM. Firstly, there are a lot of other
places where mallocFailed is set, but diag is not.
You should set this error in sqlOomFault(). Check if any
other function also can set this flag.
> + parse_context->rc = SQL_TARANTOOL_ERROR;
> return;
> }
> /*
> @@ -189,13 +190,12 @@ sql_finish_coding(struct Parse *parse_context)
> sqlVdbeGoto(v, 1);
> }
> /* Get the VDBE program ready for execution. */
> - if (parse_context->nErr == 0 && !db->mallocFailed) {
> + if (parse_context->rc == SQL_OK && !db->mallocFailed) {
> assert(parse_context->iCacheLevel == 0);
> sqlVdbeMakeReady(v, parse_context);
> - parse_context->rc = SQL_DONE;
> - } else {
> - if (parse_context->rc != SQL_TARANTOOL_ERROR)
> - parse_context->rc = SQL_ERROR;
> + } else if (parse_context->rc != SQL_TARANTOOL_ERROR){
> + diag_set(OutOfMemory, 0, "SQL", "db");
> + parse_context->rc = SQL_TARANTOOL_ERROR;
The same here.
> @@ -1284,7 +1273,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin,
> }
> sqlStartTable(parse_context, name, if_exists);
> struct space *space = parse_context->new_space;
> - if (space == NULL || parse_context->nErr != 0)
> + if (space == NULL || parse_context->rc == SQL_TARANTOOL_ERROR)
Instead of checking that rc == SQL_TARANTOOL_ERROR, I’d rather
test that rc != 0. It doesn’t matter now, since rc is anyway removed.
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 58685c4..fac2781 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");
> + pParse->rc = SQL_TARANTOOL_ERROR;
> break;
Move this change to previous patch.
> @@ -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");
> + pParse->rc = SQL_TARANTOOL_ERROR;
The same.
> @@ -529,7 +533,8 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
> #endif /* YYDEBUG */
> sqlParserFree(pEngine, sql_free);
> if (db->mallocFailed) {
> - pParse->rc = SQL_NOMEM;
> + diag_set(OutOfMemory, 0, "SQL", "db");
> + pParse->rc = SQL_TARANTOOL_ERROR;
> }
The same.
> if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE
> && pParse->zErrMsg == 0) {
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index f67c32e..a42e872 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1227,7 +1227,8 @@ valueFromFunction(sql * db, /* The database connection */
> sql_value_apply_type(pVal, type);
> assert(rc == SQL_OK);
> }
> - pCtx->pParse->rc = rc;
> + if (rc != SQL_OK)
> + pCtx->pParse->rc = SQL_TARANTOOL_ERROR;
>
Why did you change this code?
> value_from_function_out:
> if (rc != SQL_OK) {
More information about the Tarantool-patches
mailing list