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 A7513297DE for ; Mon, 18 Mar 2019 11:06:21 -0400 (EDT) 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 jiPNNMbWWTT0 for ; Mon, 18 Mar 2019 11:06:21 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 92E5629720 for ; Mon, 18 Mar 2019 11:06:20 -0400 (EDT) Date: Mon, 18 Mar 2019 18:06:17 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v4 2/8] sql: set SQL parser errors via diag_set() Message-ID: <20190318150616.GA20715@tarantool.org> References: <3be89ad14633e9b03c01200ee3d1b3c6776fccc5.1552494059.git.imeevma@gmail.com> <21543710-C2CB-4818-AE87-FA1BE3AEB0A4@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <21543710-C2CB-4818-AE87-FA1BE3AEB0A4@tarantool.org> 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" Cc: tarantool-patches@freelists.org On Thu, Mar 14, 2019 at 10:26:16PM +0300, n.pettik wrote: > > > > On 13 Mar 2019, at 20:03, imeevma@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 > > 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? > Fixed. > > } > > 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. > Fixed. > > } > > 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. > I will add this change in review fix of patch "sql: rework semantic errors". Btw I wasn't able to reproduce this error due to limit being too big. So I got an error: "error: not enough memory". Due to this I didn't create test for this error. > > + 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”). > Removed. > > + pParse->rc = SQL_TARANTOOL_ERROR; > > break; > > } > > if (tokenType == TK_ILLEGAL) { > Diff between patches: commit c5d5c541000868b1aba78d07d0b004f0022a3735 Author: Mergen Imeev Date: Fri Mar 15 20:58:48 2019 +0300 Temporary: Review fix diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c index 8812298..dc018fc 100644 --- a/src/box/sql/malloc.c +++ b/src/box/sql/malloc.c @@ -51,12 +51,13 @@ sql_sized_malloc(int nByte) assert(nByte > 0); nByte = ROUND8(nByte); p = malloc(nByte + 8); - if (p) { - p[0] = nByte; - p++; - } else { - diag_set(OutOfMemory, nByte, "realloc", "p"); + if (p == NULL) { + sql_get()->mallocFailed = 1; + diag_set(OutOfMemory, nByte, "malloc", "p"); + return NULL; } + p[0] = nByte; + p++; return (void *)p; } @@ -109,12 +110,13 @@ sql_sized_realloc(void *pPrior, int nByte) assert(nByte == ROUND8(nByte)); /* EV: R-46199-30249 */ p--; p = realloc(p, nByte + 8); - if (p) { - p[0] = nByte; - p++; - } else { - diag_set(OutOfMemory, nByte, "malloc", "p"); + if (p == NULL) { + sql_get()->mallocFailed = 1; + diag_set(OutOfMemory, nByte, "realloc", "p"); + return NULL; } + p[0] = nByte; + p++; return (void *)p; } diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 834c165..f0a2f16 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -503,12 +503,6 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (tokenType >= TK_SPACE) { assert(tokenType == TK_SPACE || tokenType == TK_ILLEGAL); - if (db->u1.isInterrupted) { - diag_set(ClientError, ER_SQL_PARSER_GENERIC, - "interrupted"); - pParse->rc = SQL_TARANTOOL_ERROR; - break; - } if (tokenType == TK_ILLEGAL) { diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, pParse->sLastToken.n, New patch: commit ded3bd139fa1fbaecf32e546d12a1ea03d69b04f Author: Mergen Imeev Date: Sun Feb 24 12:02:49 2019 +0300 sql: set SQL parser errors via diag_set() After this patch all SQL parser errors will be set via diag_set(). They were saved in field zErrMsg of struct Parse before this patch. Part of ...3965 diff --git a/src/box/errcode.h b/src/box/errcode.h index f2f47c0..d234d26 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_UNRECOGNIZED_SYNTAX, "Syntax error near '%.*s'") \ /*185 */_(ER_SQL_UNKNOWN_TOKEN, "Syntax error: unrecognized token: '%.*s'") \ + /*186 */_(ER_SQL_PARSER_GENERIC, "%s") \ /* * !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; } } /** diff --git a/src/box/sql/malloc.c b/src/box/sql/malloc.c index e0d2ec8..dc018fc 100644 --- a/src/box/sql/malloc.c +++ b/src/box/sql/malloc.c @@ -51,14 +51,13 @@ sql_sized_malloc(int nByte) assert(nByte > 0); nByte = ROUND8(nByte); p = malloc(nByte + 8); - if (p) { - p[0] = nByte; - p++; - } else { - testcase(sqlGlobalConfig.xLog != 0); - sql_log(SQL_NOMEM, - "failed to allocate %u bytes of memory", nByte); + if (p == NULL) { + sql_get()->mallocFailed = 1; + diag_set(OutOfMemory, nByte, "malloc", "p"); + return NULL; } + p[0] = nByte; + p++; return (void *)p; } @@ -111,15 +110,13 @@ sql_sized_realloc(void *pPrior, int nByte) assert(nByte == ROUND8(nByte)); /* EV: R-46199-30249 */ p--; p = realloc(p, nByte + 8); - if (p) { - 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); + if (p == NULL) { + sql_get()->mallocFailed = 1; + diag_set(OutOfMemory, nByte, "realloc", "p"); + return NULL; } + p[0] = nByte; + p++; 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..f0a2f16 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; } } else { @@ -501,10 +503,6 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (tokenType >= TK_SPACE) { assert(tokenType == TK_SPACE || tokenType == TK_ILLEGAL); - if (db->u1.isInterrupted) { - pParse->rc = SQL_INTERRUPT; - break; - } if (tokenType == TK_ILLEGAL) { diag_set(ClientError, ER_SQL_UNKNOWN_TOKEN, pParse->sLastToken.n, @@ -529,7 +527,7 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) #endif /* YYDEBUG */ sqlParserFree(pEngine, sql_free); if (db->mallocFailed) { - pParse->rc = SQL_NOMEM; + pParse->rc = SQL_TARANTOOL_ERROR; } if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE && pParse->zErrMsg == 0) { diff --git a/src/box/sql/util.c b/src/box/sql/util.c index c89e2e8..a6d1f5c 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -211,7 +211,8 @@ sqlErrorWithMsg(sql * db, int err_code, const char *zFormat, ...) } /* - * Add an error message to pParse->zErrMsg and increment pParse->nErr. + * Add an error to the diagnostics area, increment pParse->nErr + * and set pParse->rc. * The following formatting characters are allowed: * * %s Insert a string @@ -236,10 +237,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; } void diff --git a/test/box/misc.result b/test/box/misc.result index 27579c6..9f0b2c7 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -514,6 +514,7 @@ t; 182: box.error.SQL_STATEMENT_EMPTY 184: box.error.SQL_UNRECOGNIZED_SYNTAX 185: box.error.SQL_UNKNOWN_TOKEN + 186: box.error.SQL_PARSER_GENERIC ... test_run:cmd("setopt delimiter ''"); --- diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua index b6b7fc7..bab6493 100755 --- a/test/sql-tap/check.test.lua +++ b/test/sql-tap/check.test.lua @@ -319,7 +319,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "Failed to create space 'T3': SQL error: subqueries prohibited in CHECK constraints" + 1, "Failed to create space 'T3': subqueries prohibited in CHECK constraints" -- }) diff --git a/test/sql/triggers.result b/test/sql/triggers.result index 2f5b148..826e998 100644 --- a/test/sql/triggers.result +++ b/test/sql/triggers.result @@ -398,14 +398,14 @@ space_id = box.space.T1.id ... box.sql.execute("CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a = ? BEGIN SELECT 1; END;") --- -- error: 'SQL error: bindings are not allowed in DDL' +- error: bindings are not allowed in DDL ... tuple = {"TR1", space_id, {sql = [[CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a = ? BEGIN SELECT 1; END;]]}} --- ... box.space._trigger:insert(tuple) --- -- error: 'SQL error: bindings are not allowed in DDL' +- error: bindings are not allowed in DDL ... box.sql.execute("DROP TABLE t1;") ---