[tarantool-patches] Re: [PATCH v4 2/8] sql: set SQL parser errors via diag_set()

[Mergen Imeev <imeevma@tarantool.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 <imeevma@gmail.com>
> > 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 <imeevma@gmail.com>
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 <imeevma@gmail.com>
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(
         );
     ]], {
         -- <check-3.1>
-        1, "Failed to create space 'T3': SQL error: subqueries prohibited in CHECK constraints"
+        1, "Failed to create space 'T3': subqueries prohibited in CHECK constraints"
         -- </check-3.1>
     })
 
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;")
 ---