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

[Imeev Mergen]

Hi! Thank you for review! My answers below

On 2/26/19 2:01 AM, n.pettik wrote:
>
>> 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?
All unprocessed SQL errors will be inserted into the diag under
this error code.

>
>> /*
>>   * !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.
Before this change it was possible to replace SQL_TARANTOOL_ERROR
by SQL_ERROR in parse_context->rc. It actually happened in one
test.

>
>> 	}
>> }
>> /**
>> 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?
This will allow us to quickly fix issue 3505. These errors will be
processed later.

>