[tarantool-patches] Re: [PATCH v2 3/5] sql: save SQL parser errors in diag_set()

[n.pettik]

> On 25 Feb 2019, at 20:14, imeevma at tarantool.org wrote:
> 
> After this patch all SQL parser errors will be saved in diag_set()
> instead of field zErrMsg of struct Parse.
> 
> Part of #3965
> ---
> src/box/errcode.h           | 1 +
> src/box/sql/build.c         | 3 ++-
> src/box/sql/util.c          | 6 +++---
> test/box/misc.result        | 1 +
> test/sql-tap/check.test.lua | 2 +-
> test/sql/triggers.result    | 4 ++--
> 6 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 6546b2f..779d927 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -238,6 +238,7 @@ struct errcode_record {
> 	/*183 */_(ER_SQL_KEYWORD_IS_RESERVED,	"Keyword '%.*s' is reserved. Please use double quotes if '%.*s' is an identifier.") \
> 	/*184 */_(ER_SQL_SYNTAX_NEAR,		"Unrecognized syntax near '%.*s'") \
> 	/*185 */_(ER_SQL_UNKNOWN_TOKEN,		"Syntax error: unrecognized token: '%.*s'") \
> +	/*186 */_(ER_SQL_PARSER_GENERIC,	"%s") \

Hmm, what is this?

> /*
>  * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index f112c9f..deb5b89 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -194,7 +194,8 @@ sql_finish_coding(struct Parse *parse_context)
> 		sqlVdbeMakeReady(v, parse_context);
> 		parse_context->rc = SQL_DONE;
> 	} else {
> -		parse_context->rc = SQL_ERROR;
> +		if (parse_context->rc != SQL_TARANTOOL_ERROR)
> +			parse_context->rc = SQL_ERROR;

What a mess, explain this please.

> 	}
> }
> /**
> diff --git a/src/box/sql/util.c b/src/box/sql/util.c
> index c89e2e8..e4c93cb 100644
> --- a/src/box/sql/util.c
> +++ b/src/box/sql/util.c
> @@ -236,10 +236,10 @@ sqlErrorMsg(Parse * pParse, const char *zFormat, ...)
> 	va_start(ap, zFormat);
> 	zMsg = sqlVMPrintf(db, zFormat, ap);
> 	va_end(ap);
> +	diag_set(ClientError, ER_SQL_PARSER_GENERIC, zMsg);
> +	sqlDbFree(db, zMsg);
> 	pParse->nErr++;
> -	sqlDbFree(db, pParse->zErrMsg);
> -	pParse->zErrMsg = zMsg;
> -	pParse->rc = SQL_ERROR;
> +	pParse->rc = SQL_TARANTOOL_ERROR;
> }

Ok, so you decided to push all errors in one heap.
That’s definitely not what we discussed earlier.
Konstantin asked you to move each independent error
in a separate error code (and I agree with him to a lesser
or greater extent). Well, at least group them to semantic/
syntax groups. And instead you simply do this.
Then, I don’t understand purpose of all previous patches.

Am I missing something?