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 C2D7227934 for ; Tue, 26 Feb 2019 10:36:43 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qXXrwpNSszir for ; Tue, 26 Feb 2019 10:36:43 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 79BDB276C1 for ; Tue, 26 Feb 2019 10:36:43 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 4/5] sql: remove file zErrMsg of struct Parse References: <33421784356c1d83b8f38d1ff4c90982f3ad884b.1551114402.git.imeevma@gmail.com> From: Imeev Mergen Message-ID: <9df4a587-7d70-4773-ea09-63cc580e89f9@tarantool.org> Date: Tue, 26 Feb 2019 18:36:41 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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" , tarantool-patches@freelists.org Hi! Thank you for review! My answers below. On 2/26/19 5:47 PM, n.pettik wrote: >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 580f3fa..116e3e8 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -1362,13 +1362,8 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, >> parser.parse_only = true; >> >> sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, expr_list); >> - int rc = 0; >> - if (parser.rc != SQL_OK) { >> - /* Tarantool error may be already set with diag. */ >> - if (parser.rc != SQL_TARANTOOL_ERROR) >> - diag_set(ClientError, ER_SQL, parser.zErrMsg); >> - rc = -1; >> - } >> + if (parser.rc != SQL_OK) >> + return -1; > Since now we have only one possible RC, lets remove > its name and simply check (parser.rc != 0). > Or, as suggested Konstantin, better replace rc with bool is_aborted flag. Do you mind if I do this in a new patch? > >> sql_parser_destroy(&parser); >> - return rc; >> + return 0; >> } >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index deb5b89..6afca4a 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -493,16 +493,10 @@ sql_column_add_nullable_action(struct Parse *parser, >> struct field_def *field = &def->fields[def->field_count - 1]; >> if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && >> nullable_action != field->nullable_action) { >> - /* Prevent defining nullable_action many times. */ >> - const char *err_msg = >> - tt_sprintf("NULL declaration for column '%s' of table " >> - "'%s' has been already set to '%s'", >> - field->name, def->name, >> - on_conflict_action_strs[field-> >> - nullable_action]); >> - diag_set(ClientError, ER_SQL, err_msg); >> - parser->rc = SQL_TARANTOOL_ERROR; >> - parser->nErr++; >> + sqlErrorMsg(parser, "NULL declaration for column '%s' of "\ >> + "table '%s' has been already set to '%s'", >> + field->name, def->name, >> + on_conflict_action_strs[field-> nullable_action]); > This looks like step back in our attempt at using diag_set. > We do you need to incapsulate diag into sqlErrorMsg? I thought that it was good idea to incapsulate all SQL errors that cannot be fixed in just parse_context->rc = SQL_TARANTOOL_ERROR and diag_set(). Here it uses tt_printf() in addition to these two commands. > >> return; >> } >> field->nullable_action = nullable_action; >> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >> index 5170c7f..a7bf3b3 100644 >> --- a/src/box/sql/delete.c >> +++ b/src/box/sql/delete.c >> @@ -94,31 +94,22 @@ sql_table_truncate(struct Parse *parse, struct SrcList *tab_list) >> struct space *space = space_by_name(tab_name); >> if (space == NULL) { >> diag_set(ClientError, ER_NO_SUCH_SPACE, tab_name); >> - goto tarantool_error; >> + sql_parser_error(parse); > Look, anyway you remove this function in next commit. > Next time please consider order of refactoring. You are right, I will return to what was before and refactor in the next commit. > >> } >> if (! rlist_empty(&space->parent_fk_constraint)) { >> - const char *err_msg = >> - tt_sprintf("can not truncate space '%s' because other " >> - "objects depend on it", space->def->name); >> - diag_set(ClientError, ER_SQL, err_msg); >> - goto tarantool_error; >> + sqlErrorMsg(parse, "can not truncate space '%s' because other " >> + "objects depend on it", space->def->name); > Replace invocation of sqlErrorMsg with diag_set + parser->rc. > The same in other places. Answer is the same as in second question. > >> @@ -146,11 +145,7 @@ sqlPrepare(sql * db, /* Database handle. */ >> *ppStmt = (sql_stmt *) sParse.pVdbe; >> } >> >> - if (zErrMsg) { >> - sqlErrorWithMsg(db, rc, "%s", zErrMsg); >> - } else { >> - sqlError(db, rc); >> - } >> + sqlError(db, rc); > sqlError seems to be useless/dead. Please, make a note somewhere > to remove it as follow-up to error-refactoring patch-set. Do you mind if I do this in a new patch? > >