[tarantool-patches] [PATCH v3 5/9] sql: remove field zErrMsg of struct Parse
imeevma at tarantool.org
imeevma at tarantool.org
Sat Mar 2 16:07:57 MSK 2019
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 at 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
More information about the Tarantool-patches
mailing list