From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 413C5252BA for ; Tue, 26 Feb 2019 10:29:20 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ixrhYq1KJtGP for ; Tue, 26 Feb 2019 10:29:20 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id EAA682528A for ; Tue, 26 Feb 2019 10:29:19 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 3/5] sql: save SQL parser errors in diag_set() References: <85c9010267eba42be66e8c945d2b69098238a979.1551114402.git.imeevma@gmail.com> From: Imeev Mergen Message-ID: Date: Tue, 26 Feb 2019 18:29:17 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org 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@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. >