[tarantool-patches] [PATCH v3 5/9] sql: remove field zErrMsg of struct Parse

[imeevma@tarantool.org]

Hi! Thank you for review. My answers and new patch below. There
won't be diff between versions as I completely rewritten this
patch after placein it after removing of rc and nErr.

On 2/26/19 5:47 PM, n.pettik wrote:
>
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index 580f3fa..116e3e8 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -1362,13 +1362,8 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
>>  parser.parse_only = true;
>>
>>  sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, expr_list);
>> -  int rc = 0;
>> -  if (parser.rc != SQL_OK) {
>> -    /* Tarantool error may be already set with diag. */
>> -    if (parser.rc != SQL_TARANTOOL_ERROR)
>> -      diag_set(ClientError, ER_SQL, parser.zErrMsg);
>> -    rc = -1;
>> -  }
>> +  if (parser.rc != SQL_OK)
>> +    return -1;
>
> Since now we have only one possible RC, lets remove
> its name and simply check (parser.rc != 0).
> Or, as suggested Konstantin, better replace rc with bool is_aborted flag.
>
Done in previous patch.

>>  sql_parser_destroy(&parser);
>> -  return rc;
>> +  return 0;
>> }
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index deb5b89..6afca4a 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -493,16 +493,10 @@ sql_column_add_nullable_action(struct Parse *parser,
>>  struct field_def *field = &def->fields[def->field_count - 1];
>>  if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&
>>      nullable_action != field->nullable_action) {
>> -    /* Prevent defining nullable_action many times. */
>> -    const char *err_msg =
>> -      tt_sprintf("NULL declaration for column '%s' of table "
>> -           "'%s' has been already set to '%s'",
>> -           field->name, def->name,
>> -           on_conflict_action_strs[field->
>> -                 nullable_action]);
>> -    diag_set(ClientError, ER_SQL, err_msg);
>> -    parser->rc = SQL_TARANTOOL_ERROR;
>> -    parser->nErr++;
>> +    sqlErrorMsg(parser, "NULL declaration for column '%s' of "\
>> +          "table '%s' has been already set to '%s'",
>> +          field->name, def->name,
>> +          on_conflict_action_strs[field-> nullable_action]);
>
> This looks like step back in our attempt at using diag_set.
> We do you need to incapsulate diag into sqlErrorMsg?
>
Didn't include this change in new version. Actually, in a few
patches all left mentions of sqlErrorMsg() will be replaced by
tt_sprintf() + diag_set() + parser->is_aborted = true;

>>    return;
>>  }
>>  field->nullable_action = nullable_action;
>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>> index 5170c7f..a7bf3b3 100644
>> --- a/src/box/sql/delete.c
>> +++ b/src/box/sql/delete.c
>> @@ -94,31 +94,22 @@ sql_table_truncate(struct Parse *parse, struct SrcList *tab_list)
>>  struct space *space = space_by_name(tab_name);
>>  if (space == NULL) {
>>    diag_set(ClientError, ER_NO_SUCH_SPACE, tab_name);
>> -    goto tarantool_error;
>> +    sql_parser_error(parse);
>
> Look, anyway you remove this function in next commit.
> Next time please consider order of refactoring.
>
Fixed due to changed position of this patch.

>>  }
>>  if (! rlist_empty(&space->parent_fk_constraint)) {
>> -    const char *err_msg =
>> -      tt_sprintf("can not truncate space '%s' because other "
>> -           "objects depend on it", space->def->name);
>> -    diag_set(ClientError, ER_SQL, err_msg);
>> -    goto tarantool_error;
>> +    sqlErrorMsg(parse, "can not truncate space '%s' because other "
>> +          "objects depend on it", space->def->name);
>
> Replace invocation of sqlErrorMsg with diag_set + parser->rc.
> The same in other places.
>
Fixed, see my second answer.

>> @@ -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.


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);
 
  /* 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.
  *
  * This routine does NOT free the Select structure passed in.  The
  * calling function needs to do that.
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 42ff4b8..85718e1 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2641,7 +2641,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[] */
@@ -3188,7 +3187,7 @@ void sqlDequote(char *);
 void sqlNormalizeName(char *z);
 void sqlTokenInit(Token *, char *);
 int sqlKeywordCode(const unsigned char *, int);
-int sqlRunParser(Parse *, const char *, char **);
+int sqlRunParser(Parse *, const char *);
 
 /**
  * This routine is called after a single SQL statement has been
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.
  */
 int
-sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
+sqlRunParser(Parse * pParse, const char *zSql)
 {
  int nErr = 0;   /* Number of errors encountered */
  int i;      /* Loop counter */
@@ -462,7 +459,6 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
  }
  pParse->zTail = zSql;
  i = 0;
- assert(pzErrMsg != 0);
  /* sqlParserTrace(stdout, "parser: "); */
  pEngine = sqlParserAlloc(sqlMalloc);
  if (pEngine == 0) {
@@ -535,14 +531,8 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
    diag_set(OutOfMemory, 0, "SQL", "db");
    pParse->is_aborted = true;
  }
- if (pParse->is_aborted && pParse->zErrMsg == 0)
-   pParse->zErrMsg = sqlMPrintf(db, "%s", tarantoolErrorMessage());
- assert(pzErrMsg != 0);
- if (pParse->zErrMsg) {
-   *pzErrMsg = pParse->zErrMsg;
-   sql_log(SQL_TARANTOOL_ERROR, "%s", *pzErrMsg);
+ if (pParse->is_aborted)
    nErr++;
- }
  if (pParse->pVdbe != NULL && pParse->is_aborted) {
    sqlVdbeDelete(pParse->pVdbe);
    pParse->pVdbe = 0;
@@ -573,8 +563,7 @@ sql_expr_compile(sql *db, const char *expr, int expr_len)
  }
  sprintf(stmt, "%s%.*s", outer, expr_len, expr);
 
- char *sql_error = NULL;
- if (sqlRunParser(&parser, stmt, &sql_error) == SQL_OK &&
+ if (sqlRunParser(&parser, stmt) == SQL_OK &&
      parser.parsed_ast_type == AST_TYPE_EXPR) {
    expression = parser.parsed_ast.expr;
    parser.parsed_ast.expr = NULL;
@@ -593,8 +582,7 @@ sql_view_compile(struct sql *db, const char *view_stmt)
 
  struct Select *select = NULL;
 
- char *unused;
- if (sqlRunParser(&parser, view_stmt, &unused) != SQL_OK ||
+ if (sqlRunParser(&parser, view_stmt) != SQL_OK ||
      parser.parsed_ast_type != AST_TYPE_SELECT) {
    diag_set(ClientError, ER_SQL_EXECUTE, view_stmt);
  } else {
@@ -612,9 +600,8 @@ sql_trigger_compile(struct sql *db, const char *sql)
  struct Parse parser;
  sql_parser_create(&parser, db);
  parser.parse_only = true;
- char *sql_error = NULL;
  struct sql_trigger *trigger = NULL;
- if (sqlRunParser(&parser, sql, &sql_error) == SQL_OK &&
+ if (sqlRunParser(&parser, sql) == SQL_OK &&
      parser.parsed_ast_type == AST_TYPE_TRIGGER) {
    trigger = parser.parsed_ast.trigger;
    parser.parsed_ast.trigger = NULL;
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,
diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index 6129d5b..07527ff 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -211,7 +211,7 @@ sqlErrorWithMsg(sql * db, int err_code, const char *zFormat, ...)
 }
 
 /*
- * Add an error message to pParse->zErrMsg.
+ * Add an error message to diag.
  * The following formatting characters are allowed:
  *
  *      %s      Insert a string
-- 
2.7.4