[tarantool-patches] [PATCH v4 5/8] sql: remove field zErrMsg from struct Parse

[imeevma@tarantool.org]

Hi! Thank you for review! My answers and new version of patch
below. Here won't be diff between patches due to some of its code
were moved to previous patches.

On 3/5/19 12:06 PM, n.pettik wrote:
>
>>>> @@ -146,11 +145,7 @@ sqlPrepare(sql * db,  /* Database handle. */
>>>>   *ppStmt = (sql_stmt *) sParse.pVdbe;
>>>> }
>>>>
>>>> -  if (zErrMsg) {
>>>> -    sqlErrorWithMsg(db, rc, "%s", zErrMsg);
>>>> -  } else {
>>>> -    sqlError(db, rc);
>>>> -  }
>>>> +  sqlError(db, rc);
>>>
>>> sqlError seems to be useless/dead. Please, make a note somewhere
>>> to remove it as follow-up to error-refactoring patch-set.
>>>
>> Ok, made a note.
>
> And where is it? Please, paste a link to it.
I will compile all of notices to this issue and send them a bit later.

>> New version:
>>
>> commit 29c77ea3f8463994b98bcb23653f901cf46e472a
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date:   Wed Feb 27 09:40:17 2019 +0300
>>
>>    sql: remove field zErrMsg of struct Parse
>>
>>    This field become unused and should be removed.
>>
>>    Part of #3965
>>
>> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
>> index 42737ff..4359fa7 100644
>> --- a/src/box/sql/prepare.c
>> +++ b/src/box/sql/prepare.c
>> @@ -52,7 +52,6 @@ sqlPrepare(sql * db,  /* Database handle. */
>>         const char **pzTail  /* OUT: End of parsed string */
>>     )
>> {
>> - char *zErrMsg = 0;  /* Error message */
>>  int rc = SQL_OK;  /* Result code */
>>  Parse sParse;   /* Parsing context */
>>  sql_parser_create(&sParse, db);
>> @@ -89,14 +88,14 @@ sqlPrepare(sql * db,  /* Database handle. */
>>    }
>>    zSqlCopy = sqlDbStrNDup(db, zSql, nBytes);
>>    if (zSqlCopy) {
>> -     sqlRunParser(&sParse, zSqlCopy, &zErrMsg);
>> +     sqlRunParser(&sParse, zSqlCopy);
>>      sParse.zTail = &zSql[sParse.zTail - zSqlCopy];
>>      sqlDbFree(db, zSqlCopy);
>>    } else {
>>      sParse.zTail = &zSql[nBytes];
>>    }
>>  } else {
>> -   sqlRunParser(&sParse, zSql, &zErrMsg);
>> +   sqlRunParser(&sParse, zSql);
>>  }
>>  assert(0 == sParse.nQueryLoop);
>>
>> @@ -169,11 +168,7 @@ sqlPrepare(sql * db, /* Database handle. */
>>    *ppStmt = (sql_stmt *) sParse.pVdbe;
>>  }
>>
>> - if (zErrMsg) {
>> -   sqlErrorWithMsg(db, rc, "%s", zErrMsg);
>> - } else {
>> -   sqlError(db, rc);
>> - }
>> + sqlError(db, rc);
>
> As we pointed out, you can remove this call.
Removed.

>>
>>  /* Delete any TriggerPrg structures allocated while parsing this statement. */
>>  while (sParse.pTriggerPrg) {
>> @@ -318,7 +313,6 @@ sql_parser_destroy(Parse *parser)
>>    db->lookaside.bDisable -= parser->disableLookaside;
>>  }
>>  parser->disableLookaside = 0;
>> - sqlDbFree(db, parser->zErrMsg);
>>  switch (parser->parsed_ast_type) {
>>  case AST_TYPE_SELECT:
>>    sql_select_delete(db, parser->parsed_ast.select);
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index cfac553..66cbc73 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -5431,9 +5431,7 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
>>  * The results are returned according to the SelectDest structure.
>>  * See comments in sqlInt.h for further information.
>>  *
>> - * This routine returns the number of errors.  If any errors are
>> - * encountered, then an appropriate error message is left in
>> - * pParse->zErrMsg.
>> + * This routine returns the number of errors.
>
> That’s not true:
>
>         rc = pParse->is_aborted;
>
>        /* Control jumps to here if an error is encountered above, or upon
>         * successful coding of the SELECT.
>         */
>  select_end:
>        pParse->iSelectId = iRestoreSelectId;
>
>        /* Identify column names if results of the SELECT are to be output.
>         */
>        if (rc == SQL_OK && pDest->eDest == SRT_Output) {
>               generateColumnNames(pParse, pTabList, pEList);
>        }
>
>        sqlDbFree(db, sAggInfo.aCol);
>        sqlDbFree(db, sAggInfo.aFunc);
> #ifdef SQL_DEBUG
>        SELECTTRACE(1, pParse, p, ("end processing\n"));
>        pParse->nSelectIndent--;
> #endif
> return rc;
>
> So it doesn’t return count of occurred errors.
>
>> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
>> index 4ca3c2e..1a7248e 100644
>> --- a/src/box/sql/tokenize.c
>> +++ b/src/box/sql/tokenize.c
>> @@ -439,13 +439,10 @@ parser_space_delete(struct sql *db, struct space *space)
>>
>> /*
>>  * Run the parser on the given SQL string.  The parser structure is
>> - * passed in.  An SQL_ status code is returned.  If an error occurs
>> - * then an and attempt is made to write an error message into
>> - * memory obtained from sql_malloc() and to make *pzErrMsg point to that
>> - * error message.
>> + * passed in.  An SQL_ status code is returned.
>
> On the other hand:
>
> return nErr;
>
> So what is the truth?
>
Fixed, rewritten part of the comment.


New version:

commit d0d78b7eb71413f1e5b0cc7bf0c3c83e4aafe55b
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Feb 27 09:40:17 2019 +0300

    sql: remove field zErrMsg from struct Parse
    
    This field become unused and should be removed.
    
    Part of #3965

diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 85385ee..512a3dd 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -311,7 +311,6 @@ sql_parser_destroy(Parse *parser)
 		db->lookaside.bDisable -= parser->disableLookaside;
 	}
 	parser->disableLookaside = 0;
-	sqlDbFree(db, parser->zErrMsg);
 	switch (parser->parsed_ast_type) {
 	case AST_TYPE_SELECT:
 		sql_select_delete(db, parser->parsed_ast.select);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index e83b898..9a8ea9d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5431,12 +5431,11 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
  * The results are returned according to the SelectDest structure.
  * See comments in sqlInt.h for further information.
  *
- * This routine returns the number of errors.  If any errors are
- * encountered, then an appropriate error message is left in
- * pParse->zErrMsg.
- *
  * This routine does NOT free the Select structure passed in.  The
  * calling function needs to do that.
+ *
+ * @retval 0 on success.
+ * @retval != 0 on error.
  */
 int
 sqlSelect(Parse * pParse,		/* The parser context */
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b05e67b..5ddaca2 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2653,7 +2653,6 @@ struct fk_constraint_parse {
  */
 struct Parse {
 	sql *db;		/* The main database structure */
-	char *zErrMsg;		/* An error message */
 	Vdbe *pVdbe;		/* An engine for executing database bytecode */
 	u8 colNamesSet;		/* TRUE after OP_ColumnName has been issued to pVdbe */
 	u8 nTempReg;		/* Number of temporary registers in aTempReg[] */
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index be04c17..db06c39 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -531,8 +531,6 @@ sqlRunParser(Parse * pParse, const char *zSql)
 	sqlParserFree(pEngine, sql_free);
 	if (db->mallocFailed)
 		pParse->is_aborted = true;
-	if (pParse->is_aborted && pParse->zErrMsg == 0)
-		pParse->zErrMsg = sqlMPrintf(db, "%s", tarantoolErrorMessage());
 	if (pParse->pVdbe != NULL && pParse->is_aborted) {
 		sqlVdbeDelete(pParse->pVdbe);
 		pParse->pVdbe = 0;
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 7eacd33..2f1268a 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -721,25 +721,6 @@ onErrorText(int onError)
 }
 #endif
 
-/*
- * Parse context structure pFrom has just been used to create a sub-vdbe
- * (trigger program). If an error has occurred, transfer error information
- * from pFrom to pTo.
- */
-static void
-transferParseError(Parse * pTo, Parse * pFrom)
-{
-	assert(pFrom->zErrMsg == 0 || pFrom->is_aborted);
-	assert(pTo->zErrMsg == 0 || pTo->is_aborted);
-	if (!pTo->is_aborted) {
-		pTo->zErrMsg = pFrom->zErrMsg;
-		pTo->is_aborted = pFrom->is_aborted;
-	} else {
-		sqlDbFree(pFrom->db, pFrom->zErrMsg);
-	}
-	pFrom->zErrMsg = NULL;
-}
-
 /**
  * Create and populate a new TriggerPrg object with a sub-program
  * implementing trigger pTrigger with ON CONFLICT policy orconf.
@@ -848,7 +829,8 @@ sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
 		VdbeComment((v, "End: %s.%s", trigger->zName,
 			     onErrorText(orconf)));
 
-		transferParseError(parser, pSubParse);
+		if (!parser->is_aborted)
+			parser->is_aborted = pSubParse->is_aborted;
 		if (db->mallocFailed == 0) {
 			pProgram->aOp =
 			    sqlVdbeTakeOpArray(v, &pProgram->nOp,
-- 
2.7.4