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 45D5C2981A for ; Mon, 18 Mar 2019 11:28:09 -0400 (EDT) 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 h4hVMmN7K1tI for ; Mon, 18 Mar 2019 11:28:09 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 E8F9C29811 for ; Mon, 18 Mar 2019 11:28:07 -0400 (EDT) Date: Mon, 18 Mar 2019 18:28:04 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse Message-ID: <20190318152804.GA24675@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: "n.pettik" Cc: tarantool-patches@freelists.org Hi! Thank you for review. My answers and two new patches below. On Thu, Mar 14, 2019 at 10:53:00PM +0300, n.pettik wrote: > > >>> diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > >>> index bab6493..0d8bf15 100755 > >>> --- a/test/sql-tap/check.test.lua > >>> +++ b/test/sql-tap/check.test.lua > >>> @@ -516,7 +516,7 @@ test:do_catchsql_test( > >>> ); > >>> ]], { > >>> -- > >>> - 1, "Wrong space options (field 5): invalid expression specified (SQL error: bindings are not allowed in DDL)" > >>> + 1, "Wrong space options (field 5): invalid expression specified (bindings are not allowed in DDL)" > >>> -- > >>> }) > >> > >> Why test results have changed if you provided > >> non-functional refactoring? > > It become this way because now the error in diag instead of being > > only in zErrMsg of struct Parse. > > So, then it should be related to the previous patch, I guess. > Otherwise, still don’t understand. Or fix commit message, > since now it implies that only refactoring was provided. > Or, what is better - move functional changes to separate patch. > I divided this ptch into two: "sql: remove argument pzErrMsg from sqlRunParser()" "sql: replace rc with is_aborted status in struct Parse" > > index a2937a0..c2e5d6b 100644 > > --- a/src/box/sql.c > > +++ b/src/box/sql.c > > @@ -1363,12 +1363,8 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, > > > > 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); > > + if (parser.is_aborted) > > rc = -1; > > - } > > sql_parser_destroy(&parser); > > return rc; > > diff --git a/src/box/sql.c b/src/box/sql.c > index c2e5d6bd1..ea71dd101 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1362,9 +1362,6 @@ 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.is_aborted) > - rc = -1; > sql_parser_destroy(&parser); > - return rc; > + return parser.is_aborted ? -1 : 0; > } > I am not sure that this should be applied. I think it isn't right to look at field is_aborted of parser after parser destruction. > > } > > > > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > > index 828a1ae..85385ee 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,25 +88,24 @@ 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); > > > > - if (sParse.rc == SQL_DONE) > > - sParse.rc = SQL_OK; > > if (db->mallocFailed) > > - sParse.rc = SQL_TARANTOOL_ERROR; > > + sParse.is_aborted = true; > > if (pzTail) { > > *pzTail = sParse.zTail; > > } > > - rc = sParse.rc; > > + if (sParse.is_aborted) > > + rc = SQL_TARANTOOL_ERROR; > > > > if (rc == SQL_OK && sParse.pVdbe && sParse.explain) { > > static const char *const azColName[] = { > > @@ -168,11 +166,7 @@ sqlPrepare(sql * db, /* Database handle. */ > > *ppStmt = (sql_stmt *) sParse.pVdbe; > > } > > > > - if (zErrMsg) { > > - sqlErrorWithMsg(db, rc, "%s", zErrMsg); > > - } else { > > - sqlError(db, rc); > > - } > > + db->errCode = rc; > > Is this thing used anywhere? Yes, it is used in sql_errmsg(). It contains VDBE errors too, so I think it would be better to leave it for now. > > First new patch: commit 50af26719f6d195417f546ea2cfd4e70e3b09421 Author: Mergen Imeev Date: Fri Mar 15 21:37:58 2019 +0300 sql: remove argument pzErrMsg from sqlRunParser() This argument has practically no functionality, but deleting it allows us to replace the rc field of the Parse structure with a new bool field. Part of #3965 diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 828a1ae..4eaa9a4 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); @@ -168,11 +167,7 @@ sqlPrepare(sql * db, /* Database handle. */ *ppStmt = (sql_stmt *) sParse.pVdbe; } - if (zErrMsg) { - sqlErrorWithMsg(db, rc, "%s", zErrMsg); - } else { - sqlError(db, rc); - } + db->errCode = rc; /* Delete any TriggerPrg structures allocated while parsing this statement. */ while (sParse.pTriggerPrg) { diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index c63ff1c..b9b45b5 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -3198,7 +3198,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 *); /** * Increment error counter. diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index f0a2f16..8ee996e 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -437,17 +437,17 @@ parser_space_delete(struct sql *db, struct space *space) sql_expr_list_delete(db, space->def->opts.checks); } -/* - * 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. +/** + * Run the parser on the given SQL string. + * + * @param pParse Parser context. + * @param zSql SQL string. + * @retval 0 on success. + * @retval -1 on error. */ 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 */ void *pEngine; /* The LEMON-generated LALR(1) parser */ int tokenType; /* type of the next token */ @@ -463,12 +463,11 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) pParse->rc = SQL_OK; pParse->zTail = zSql; i = 0; - assert(pzErrMsg != 0); /* sqlParserTrace(stdout, "parser: "); */ pEngine = sqlParserAlloc(sqlMalloc); if (pEngine == 0) { sqlOomFault(db); - return SQL_NOMEM; + return -1; } assert(pParse->new_space == NULL); assert(pParse->parsed_ast.trigger == NULL); @@ -518,7 +517,6 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) break; } } - assert(nErr == 0); pParse->zTail = &zSql[i]; #ifdef YYTRACKMAXSTACKDEPTH sqlStatusHighwater(SQL_STATUS_PARSER_STACK, @@ -530,21 +528,8 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) pParse->rc = SQL_TARANTOOL_ERROR; } if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE - && pParse->zErrMsg == 0) { - const char *error; - if (is_tarantool_error(pParse->rc) && - tarantoolErrorMessage() != NULL) - error = tarantoolErrorMessage(); - else - error = sqlErrStr(pParse->rc); - pParse->zErrMsg = sqlMPrintf(db, "%s", error); - } - assert(pzErrMsg != 0); - if (pParse->zErrMsg) { - *pzErrMsg = pParse->zErrMsg; - sql_log(pParse->rc, "%s", *pzErrMsg); - nErr++; - } + && pParse->zErrMsg == 0) + pParse->zErrMsg = sqlMPrintf(db, "%s", tarantoolErrorMessage()); if (pParse->pVdbe != NULL && pParse->nErr > 0) { sqlVdbeDelete(pParse->pVdbe); pParse->pVdbe = 0; @@ -553,8 +538,7 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) if (pParse->pWithToFree) sqlWithDelete(db, pParse->pWithToFree); sqlDbFree(db, pParse->pVList); - assert(nErr == 0 || pParse->rc != SQL_OK); - return nErr; + return pParse->nErr > 0 || pParse->rc == SQL_TARANTOOL_ERROR ? -1 : 0; } struct Expr * @@ -575,11 +559,8 @@ 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 || - parser.parsed_ast_type != AST_TYPE_EXPR) { - diag_set(ClientError, ER_SQL, sql_error); - } else { + if (sqlRunParser(&parser, stmt) == 0 && + parser.parsed_ast_type == AST_TYPE_EXPR) { expression = parser.parsed_ast.expr; parser.parsed_ast.expr = NULL; } @@ -597,8 +578,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) != 0 || parser.parsed_ast_type != AST_TYPE_SELECT) { diag_set(ClientError, ER_SQL_EXECUTE, view_stmt); } else { @@ -616,13 +596,9 @@ 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 || - parser.parsed_ast_type != AST_TYPE_TRIGGER) { - if (parser.rc != SQL_TARANTOOL_ERROR) - diag_set(ClientError, ER_SQL, sql_error); - } else { + if (sqlRunParser(&parser, sql) == 0 && + parser.parsed_ast_type == AST_TYPE_TRIGGER) { trigger = parser.parsed_ast.trigger; parser.parsed_ast.trigger = NULL; } diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua index bab6493..0d8bf15 100755 --- a/test/sql-tap/check.test.lua +++ b/test/sql-tap/check.test.lua @@ -516,7 +516,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "Wrong space options (field 5): invalid expression specified (SQL error: bindings are not allowed in DDL)" + 1, "Wrong space options (field 5): invalid expression specified (bindings are not allowed in DDL)" -- }) @@ -528,7 +528,7 @@ test:do_catchsql_test( ); ]], { -- - 1, "Wrong space options (field 5): invalid expression specified (SQL error: bindings are not allowed in DDL)" + 1, "Wrong space options (field 5): invalid expression specified (bindings are not allowed in DDL)" -- }) diff --git a/test/sql/checks.result b/test/sql/checks.result index e31964c..42df657 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -29,8 +29,8 @@ t = {513, 1, 'test', 'memtx', 0, opts, format} ... s = box.space._space:insert(t) --- -- error: 'Wrong space options (field 5): invalid expression specified (SQL error: - Syntax error near ''<'')' +- error: 'Wrong space options (field 5): invalid expression specified (Syntax error + near ''<'')' ... opts = {checks = {{expr = 'X>5'}}} --- @@ -122,8 +122,8 @@ box.sql.execute("DROP TABLE w2;") -- box.sql.execute("CREATE TABLE t5(x INT PRIMARY KEY, y INT, CHECK( x*y < ? ));") --- -- error: 'Wrong space options (field 5): invalid expression specified (SQL error: - bindings are not allowed in DDL)' +- error: 'Wrong space options (field 5): invalid expression specified (bindings are + not allowed in DDL)' ... opts = {checks = {{expr = '?>5', name = 'ONE'}}} --- @@ -136,8 +136,8 @@ t = {513, 1, 'test', 'memtx', 0, opts, format} ... s = box.space._space:insert(t) --- -- error: 'Wrong space options (field 5): invalid expression specified (SQL error: - bindings are not allowed in DDL)' +- error: 'Wrong space options (field 5): invalid expression specified (bindings are + not allowed in DDL)' ... test_run:cmd("clear filter") --- Second new patch: commit c8316b7f20ffad80857568fab2ad422c85218d05 Author: Mergen Imeev Date: Wed Mar 6 22:09:26 2019 +0300 sql: replace rc with is_aborted status in struct Parse Currently, field representing return code in struct Parse can take only two values: SQL_OK (successfully finished parsing) and SQL_TARANTOOL_ERROR (in case of any errors occurred). Therefore, it can be replaced with a boolean field. Patch provides straightforward refactoring. Part of #3965 diff --git a/src/box/sql.c b/src/box/sql.c index a2937a0..c2e5d6b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1275,7 +1275,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name) if (def == NULL) { diag_set(OutOfMemory, size, "region_alloc", "sql_ephemeral_space_def_new"); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return NULL; } @@ -1294,7 +1294,7 @@ sql_ephemeral_space_new(Parse *parser, const char *name) struct space *space = (struct space *) region_alloc(&parser->region, sz); if (space == NULL) { diag_set(OutOfMemory, sz, "region", "space"); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return NULL; } @@ -1363,12 +1363,8 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, 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); + if (parser.is_aborted) rc = -1; - } sql_parser_destroy(&parser); return rc; } diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index d49ebb8..fe4754f 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -72,7 +72,7 @@ exit_rename_table: return; tnt_error: sqlDbFree(db, new_name); - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; goto exit_rename_table; } diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index ea5cbc3..96b7099 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -907,7 +907,7 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space) if (jump_addrs == NULL) { diag_set(OutOfMemory, sizeof(int) * part_count, "region", "jump_addrs"); - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; return; } @@ -1130,7 +1130,7 @@ sqlAnalyze(Parse * pParse, Token * pName) } } else { diag_set(ClientError, ER_NO_SUCH_SPACE, z); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; } sqlDbFree(db, z); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index deb5b89..0179a45 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -91,7 +91,7 @@ save_record(struct Parse *parser, uint32_t space_id, int reg_key, if (record == NULL) { diag_set(OutOfMemory, sizeof(*record), "region_alloc", "record"); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return; } @@ -146,8 +146,7 @@ sql_finish_coding(struct Parse *parse_context) } if (db->mallocFailed || parse_context->nErr != 0) { - if (parse_context->rc == SQL_OK) - parse_context->rc = SQL_ERROR; + parse_context->is_aborted = true; return; } /* @@ -192,10 +191,8 @@ sql_finish_coding(struct Parse *parse_context) if (parse_context->nErr == 0 && !db->mallocFailed) { assert(parse_context->iCacheLevel == 0); sqlVdbeMakeReady(v, parse_context); - parse_context->rc = SQL_DONE; } else { - if (parse_context->rc != SQL_TARANTOOL_ERROR) - parse_context->rc = SQL_ERROR; + parse_context->is_aborted = true; } } /** @@ -397,7 +394,7 @@ sql_field_retrieve(Parse *parser, struct space_def *space_def, uint32_t id) diag_set(OutOfMemory, columns_new * sizeof(space_def->fields[0]), "region_alloc", "sql_field_retrieve"); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return NULL; } @@ -453,7 +450,7 @@ sqlAddColumn(Parse * pParse, Token * pName, struct type_def *type_def) if (z == NULL) { diag_set(OutOfMemory, pName->n + 1, "region_alloc", "z"); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; return; } @@ -501,7 +498,7 @@ sql_column_add_nullable_action(struct Parse *parser, on_conflict_action_strs[field-> nullable_action]); diag_set(ClientError, ER_SQL, err_msg); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return; } @@ -543,7 +540,7 @@ sqlAddDefaultValue(Parse * pParse, ExprSpan * pSpan) diag_set(OutOfMemory, default_length + 1, "region_alloc", "field->default_value"); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; return; } @@ -562,7 +559,7 @@ field_def_create_for_pk(struct Parse *parser, struct field_def *field, if (field->nullable_action != ON_CONFLICT_ACTION_ABORT && field->nullable_action != ON_CONFLICT_ACTION_DEFAULT) { diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return -1; } else if (field->nullable_action == ON_CONFLICT_ACTION_DEFAULT) { @@ -851,7 +848,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def, save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1); return; error: - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; } @@ -912,7 +909,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt) return; error: pParse->nErr++; - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; } int @@ -1093,7 +1090,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, return; error: parse_context->nErr++; - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; } /** @@ -1121,7 +1118,7 @@ resolve_link(struct Parse *parse_context, const struct space_def *def, diag_set(ClientError, ER_CREATE_FK_CONSTRAINT, fk_name, tt_sprintf("unknown column %s in foreign key definition", field_name)); - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; return -1; } @@ -1253,7 +1250,7 @@ sqlEndTable(Parse * pParse, /* Parse context */ "foreign key does not match the "\ "number of columns in the primary "\ "index of referenced table"); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; return; } @@ -1329,7 +1326,7 @@ sql_create_view(struct Parse *parse_context, struct Token *begin, space->def->opts.sql = strndup(begin->z, n); if (space->def->opts.sql == NULL) { diag_set(OutOfMemory, n, "strndup", "opts.sql"); - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; goto create_view_fail; } @@ -1649,7 +1646,7 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list, if (! fk_constraint_is_self_referenced(fk->def)) { diag_set(ClientError, ER_DROP_SPACE, space_name, "other objects depend on it"); - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; goto exit_drop_table; } @@ -1685,7 +1682,7 @@ columnno_by_name(struct Parse *parse_context, const struct space *space, diag_set(ClientError, ER_CREATE_FK_CONSTRAINT, fk_name, tt_sprintf("foreign key refers to nonexistent field %s", column_name)); - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; return -1; } @@ -1895,7 +1892,7 @@ exit_create_fk: sqlDbFree(db, constraint_name); return; tnt_error: - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; goto exit_create_fk; } @@ -1920,7 +1917,7 @@ sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table, struct space *child = space_by_name(table_name); if (child == NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; parse_context->nErr++; return; } @@ -2099,7 +2096,7 @@ cleanup: key_def_delete(key_def); return rc; tnt_error: - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; ++parse->nErr; goto cleanup; } @@ -2150,7 +2147,7 @@ sql_create_index(struct Parse *parse, struct Token *token, if (space == NULL) { if (! if_not_exist) { diag_set(ClientError, ER_NO_SUCH_SPACE, name); - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; } goto exit_create_index; @@ -2245,7 +2242,7 @@ sql_create_index(struct Parse *parse, struct Token *token, diag_set(ClientError, ER_MODIFY_INDEX, name, def->name, "can't create index on system space"); parse->nErr++; - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; goto exit_create_index; } @@ -2273,7 +2270,7 @@ sql_create_index(struct Parse *parse, struct Token *token, index = (struct index *) region_alloc(&parse->region, sizeof(*index)); if (index == NULL) { diag_set(OutOfMemory, sizeof(*index), "region", "index"); - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; goto exit_create_index; } diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c index 4594cac..5c9cf98 100644 --- a/src/box/sql/callback.c +++ b/src/box/sql/callback.c @@ -49,7 +49,7 @@ sql_get_coll_seq(Parse *parser, const char *name, uint32_t *coll_id) struct coll_id *p = coll_by_name(name, strlen(name)); if (p == NULL) { diag_set(ClientError, ER_NO_SUCH_COLLATION, name); - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return NULL; } else { diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 5170c7f..3123681 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -50,7 +50,7 @@ sql_lookup_space(struct Parse *parse, struct SrcList_item *space_name) if (space->def->field_count == 0) { diag_set(ClientError, ER_UNSUPPORTED, "SQL", "space without format"); - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; return NULL; } @@ -116,7 +116,7 @@ cleanup: return; tarantool_error: - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; goto cleanup; } diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 82688df..39b747d 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -258,7 +258,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id, diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX); parse->nErr++; - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; } return -1; } @@ -433,7 +433,7 @@ sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, return -1; if (collations_check_compatibility(lhs_coll_id, is_lhs_forced, rhs_coll_id, is_rhs_forced, id) != 0) { - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return -1; } diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 8b909f2..d8a0bda 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -506,7 +506,7 @@ sqlPragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ box_iterator_t* iter; iter = box_index_iterator(space->def->id, 0,ITER_ALL, key_buf, key_end); if (iter == NULL) { - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; goto pragma_out; } @@ -565,7 +565,7 @@ sqlPragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ if (!token_is_string(pValue)) { diag_set(ClientError, ER_ILLEGAL_PARAMS, "string value is expected"); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; goto pragma_out; } @@ -577,7 +577,7 @@ sqlPragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ sqlVdbeAddOp2(v, OP_ResultRow, 1, 1); } else { if (sql_default_engine_set(zRight) != 0) { - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; pParse->nErr++; goto pragma_out; } diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index 4eaa9a4..85385ee 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -99,14 +99,13 @@ sqlPrepare(sql * db, /* Database handle. */ } assert(0 == sParse.nQueryLoop); - if (sParse.rc == SQL_DONE) - sParse.rc = SQL_OK; if (db->mallocFailed) - sParse.rc = SQL_TARANTOOL_ERROR; + sParse.is_aborted = true; if (pzTail) { *pzTail = sParse.zTail; } - rc = sParse.rc; + if (sParse.is_aborted) + rc = SQL_TARANTOOL_ERROR; if (rc == SQL_OK && sParse.pVdbe && sParse.explain) { static const char *const azColName[] = { diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index 49b0052..5b9d216 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -1341,7 +1341,7 @@ resolveSelectStep(Walker * pWalker, Select * p) "clause or be used in an aggregate " "function"); pParse->nErr++; - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; return WRC_Abort; } /* diff --git a/src/box/sql/select.c b/src/box/sql/select.c index ef24760..7185a10 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -2235,7 +2235,7 @@ multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n, if (collations_check_compatibility(prior_coll_id, is_prior_forced, current_coll_id, is_current_forced, &res_coll_id) != 0) { - parser->rc = SQL_TARANTOOL_ERROR; + parser->is_aborted = true; parser->nErr++; return 0; } diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index b9b45b5..22fab27 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2653,7 +2653,6 @@ struct Parse { sql *db; /* The main database structure */ char *zErrMsg; /* An error message */ Vdbe *pVdbe; /* An engine for executing database bytecode */ - int rc; /* Return code from execution */ u8 colNamesSet; /* TRUE after OP_ColumnName has been issued to pVdbe */ u8 nTempReg; /* Number of temporary registers in aTempReg[] */ u8 isMultiWrite; /* True if statement may modify/insert multiple rows */ @@ -2688,6 +2687,8 @@ struct Parse { u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ /** Region to make SQL temp allocations. */ struct region region; + /** True, if error should be raised after parsing. */ + bool is_aborted; /************************************************************************** * Fields above must be initialized to zero. The fields that follow, diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 8ee996e..00a84ab 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -460,7 +460,6 @@ sqlRunParser(Parse * pParse, const char *zSql) if (db->nVdbeActive == 0) { db->u1.isInterrupted = 0; } - pParse->rc = SQL_OK; pParse->zTail = zSql; i = 0; /* sqlParserTrace(stdout, "parser: "); */ @@ -484,7 +483,7 @@ sqlRunParser(Parse * pParse, const char *zSql) if (i > mxSqlLen) { diag_set(ClientError, ER_SQL_PARSER_GENERIC, "string or blob too big"); - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; break; } } else { @@ -513,7 +512,7 @@ sqlRunParser(Parse * pParse, const char *zSql) sqlParser(pEngine, tokenType, pParse->sLastToken, pParse); lastTokenParsed = tokenType; - if (pParse->rc != SQL_OK || db->mallocFailed) + if (pParse->is_aborted || db->mallocFailed) break; } } @@ -524,11 +523,9 @@ sqlRunParser(Parse * pParse, const char *zSql) ); #endif /* YYDEBUG */ sqlParserFree(pEngine, sql_free); - if (db->mallocFailed) { - pParse->rc = SQL_TARANTOOL_ERROR; - } - if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE - && pParse->zErrMsg == 0) + 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->nErr > 0) { sqlVdbeDelete(pParse->pVdbe); @@ -538,7 +535,7 @@ sqlRunParser(Parse * pParse, const char *zSql) if (pParse->pWithToFree) sqlWithDelete(db, pParse->pWithToFree); sqlDbFree(db, pParse->pVList); - return pParse->nErr > 0 || pParse->rc == SQL_TARANTOOL_ERROR ? -1 : 0; + return pParse->is_aborted ? -1 : 0; } struct Expr * diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index f7e6189..5ee0d96 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -154,7 +154,7 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm, return; set_tarantool_error_and_cleanup: - parse->rc = SQL_TARANTOOL_ERROR; + parse->is_aborted = true; parse->nErr++; goto trigger_cleanup; } @@ -735,7 +735,7 @@ transferParseError(Parse * pTo, Parse * pFrom) if (pTo->nErr == 0) { pTo->zErrMsg = pFrom->zErrMsg; pTo->nErr = pFrom->nErr; - pTo->rc = pFrom->rc; + pTo->is_aborted = pFrom->is_aborted; } else { sqlDbFree(pFrom->db, pFrom->zErrMsg); } diff --git a/src/box/sql/util.c b/src/box/sql/util.c index a6d1f5c..5aa4fda 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -212,7 +212,7 @@ sqlErrorWithMsg(sql * db, int err_code, const char *zFormat, ...) /* * Add an error to the diagnostics area, increment pParse->nErr - * and set pParse->rc. + * and set pParse->is_aborted. * The following formatting characters are allowed: * * %s Insert a string @@ -240,14 +240,14 @@ sqlErrorMsg(Parse * pParse, const char *zFormat, ...) diag_set(ClientError, ER_SQL_PARSER_GENERIC, zMsg); sqlDbFree(db, zMsg); pParse->nErr++; - pParse->rc = SQL_TARANTOOL_ERROR; + pParse->is_aborted = true; } void sql_parser_error(struct Parse *parse_context) { parse_context->nErr++; - parse_context->rc = SQL_TARANTOOL_ERROR; + parse_context->is_aborted = true; } /* diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 074ff8c..f417c49 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -1203,7 +1203,7 @@ valueFromFunction(sql * db, /* The database connection */ goto value_from_function_out; } - assert(pCtx->pParse->rc == SQL_OK); + assert(!pCtx->pParse->is_aborted); memset(&ctx, 0, sizeof(ctx)); ctx.pOut = pVal; ctx.pFunc = pFunc; @@ -1215,7 +1215,8 @@ valueFromFunction(sql * db, /* The database connection */ sql_value_apply_type(pVal, type); assert(rc == SQL_OK); } - pCtx->pParse->rc = rc; + if (rc != SQL_OK) + pCtx->pParse->is_aborted = true; value_from_function_out: if (rc != SQL_OK) { diff --git a/src/box/sql/where.c b/src/box/sql/where.c index b46b7c3..3e1c783 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -2800,7 +2800,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */ if (key_def == NULL) { tnt_error: pWInfo->pParse->nErr++; - pWInfo->pParse->rc = SQL_TARANTOOL_ERROR; + pWInfo->pParse->is_aborted = true; return SQL_TARANTOOL_ERROR; }