[tarantool-patches] Re: [PATCH v4 3/8] sql: replace rc with is_aborted status in struct Parse
Kirill Yukhin
kyukhin at tarantool.org
Tue Mar 19 16:17:44 MSK 2019
Hello,
On 13 Mar 20:03, imeevma at 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(
> >> );
> >> ]], {
> >> -- <check-5.1>
> >> - 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)"
> >> -- </check-5.1>
> >> })
> >
> > 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 <imeevma at gmail.com>
> 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(
> );
> ]], {
> -- <check-5.1>
> - 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)"
> -- </check-5.1>
> })
>
> @@ -528,7 +528,7 @@ test:do_catchsql_test(
> );
> ]], {
> -- <check-5.2>
> - 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)"
> -- </check-5.2>
> })
>
> 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
>
>
More information about the Tarantool-patches
mailing list