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 10E6429400 for ; Tue, 19 Mar 2019 09:17:49 -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 inBT1e58C1M4 for ; Tue, 19 Mar 2019 09:17:48 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 4EAFB293E9 for ; Tue, 19 Mar 2019 09:17:48 -0400 (EDT) Date: Tue, 19 Mar 2019 16:17:44 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse Message-ID: <20190319131744.f4k5ugnvmeiphums@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: tarantool-patches@freelists.org Cc: korablev@tarantool.org Hello, On 13 Mar 20:03, imeevma@tarantool.org wrote: > Hi! Thank you for review! My answers and new version of patch > below. Here won't be diff between patches as I rewritten this > patch to change its positions with one that removes nErr. I've checked your patch into 2.1 branch (added Nikita's nit > > On 3/5/19 12:06 PM, n.pettik wrote: > > Nit: remove from. > Fixed. > > >> Currently, the field rc of the struct Parse can have only two > >> values: SQL_OK or SQL_TARANTOOL_ERROR. Therefore, it is logical to > >> replace it with a new boolean field. This patche replaces field rc > >> by new field is_aborted. > > > > Fixed commit message (original one contained several mistakes): > > > > sql: replace rc with is_abort status in stuct 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 > Thanks, fixed. > > >> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > >> index 0c6296d..42737ff 100644 > >> --- a/src/box/sql/prepare.c > >> +++ b/src/box/sql/prepare.c > >> @@ -100,15 +100,15 @@ sqlPrepare(sql * db, /* Database handle. */ > >> } > >> assert(0 == sParse.nQueryLoop); > >> > >> - if (sParse.rc == SQL_DONE) > >> - sParse.rc = SQL_OK; > >> if (db->mallocFailed) { > >> - sParse.rc = SQL_NOMEM; > >> + diag_set(OutOfMemory, 0, "SQL", "db"); > >> + sParse.is_aborted = true; > > > > See comments for previous patches. > Fixed in patch "sql: set SQL parser errors via diag_set()". > > >> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > >> index 719b85e..42ff4b8 100644 > >> --- a/src/box/sql/sqlInt.h > >> +++ b/src/box/sql/sqlInt.h > >> @@ -2643,7 +2643,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 */ > >> @@ -2677,6 +2676,8 @@ struct Parse { > >> u8 eOrconf; /* Default ON CONFLICT policy for trigger steps */ > >> /** Region to make SQL temp allocations. */ > >> struct region region; > >> + /** Flag to show that parsing should be aborted. */ > > > > Comment is misleading: now we don’t abort parsing process, > > but instead allow it to be finished and raise an error at the end. > > Fix comment pls. > Fixed. > > >> + bool is_aborted; > >> > >> /************************************************************************** > >> * Fields above must be initialized to zero. The fields that follow, > >> @@ -534,25 +533,17 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > >> sqlParserFree(pEngine, sql_free); > >> if (db->mallocFailed) { > >> diag_set(OutOfMemory, 0, "SQL", "db"); > >> - 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); > >> + 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(pParse->rc, "%s", *pzErrMsg); > >> + sql_log(SQL_TARANTOOL_ERROR, "%s", *pzErrMsg); > > > > Do we need this call at all? > Fixed, removed. > > >> 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. > > > New version: > > commit ad9f22e790f24598fae717ff1de8992b43ef48c9 > 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 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; > > /* Delete any TriggerPrg structures allocated while parsing this statement. */ > while (sParse.pTriggerPrg) { > 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 eb14885..dd21091 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -2655,7 +2655,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 */ > @@ -2690,6 +2689,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, > @@ -3200,7 +3201,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 834c165..5c23ec0 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 */ > @@ -460,15 +460,13 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > if (db->nVdbeActive == 0) { > db->u1.isInterrupted = 0; > } > - 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); > @@ -485,7 +483,7 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > 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 { > @@ -506,7 +504,7 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > if (db->u1.isInterrupted) { > diag_set(ClientError, ER_SQL_PARSER_GENERIC, > "interrupted"); > - pParse->rc = SQL_TARANTOOL_ERROR; > + pParse->is_aborted = true; > break; > } > if (tokenType == TK_ILLEGAL) { > @@ -520,11 +518,10 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > sqlParser(pEngine, tokenType, pParse->sLastToken, > pParse); > lastTokenParsed = tokenType; > - if (pParse->rc != SQL_OK || db->mallocFailed) > + if (pParse->is_aborted || db->mallocFailed) > break; > } > } > - assert(nErr == 0); > pParse->zTail = &zSql[i]; > #ifdef YYTRACKMAXSTACKDEPTH > sqlStatusHighwater(SQL_STATUS_PARSER_STACK, > @@ -532,25 +529,10 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > ); > #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) { > - 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++; > - } > + 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); > pParse->pVdbe = 0; > @@ -559,8 +541,9 @@ 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; > + if (pParse->is_aborted) > + return -1; > + return 0; > } > > struct Expr * > @@ -581,11 +564,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; > } > @@ -603,8 +583,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 { > @@ -622,13 +601,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/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 5a3c9be..6c5c61e 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; > } > > 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") > --- > -- > 2.7.4 > >