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 229A026208 for ; Sat, 2 Mar 2019 08:08:00 -0500 (EST) 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 KHbBiQofLNke for ; Sat, 2 Mar 2019 08:08:00 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 90CFD261E0 for ; Sat, 2 Mar 2019 08:07:59 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v3 5/9] sql: remove field zErrMsg of struct Parse Date: Sat, 2 Mar 2019 16:07:57 +0300 Message-Id: <29c77ea3f8463994b98bcb23653f901cf46e472a.1551530224.git.imeevma@gmail.com> In-Reply-To: References: 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: korablev@tarantool.org Cc: tarantool-patches@freelists.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 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