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 940D02687F for ; Tue, 26 Feb 2019 13:17:40 -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 GJFFZdwAbOsP for ; Tue, 26 Feb 2019 13:17:40 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 250DB2682C for ; Tue, 26 Feb 2019 13:17:40 -0500 (EST) From: "n.pettik" Message-Id: <6A8368F5-2B85-4CC0-9647-EEF575DEB6CA@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_276A8A4A-A052-4A14-9387-DD1FCAF29400" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 4/5] sql: remove file zErrMsg of struct Parse Date: Tue, 26 Feb 2019 21:17:37 +0300 In-Reply-To: <9df4a587-7d70-4773-ea09-63cc580e89f9@tarantool.org> References: <33421784356c1d83b8f38d1ff4c90982f3ad884b.1551114402.git.imeevma@gmail.com> <9df4a587-7d70-4773-ea09-63cc580e89f9@tarantool.org> 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: Imeev Mergen --Apple-Mail=_276A8A4A-A052-4A14-9387-DD1FCAF29400 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 26 Feb 2019, at 18:36, Imeev Mergen wrote: > 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 =3D true; >>>=20 >>> sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, = expr_list); >>> - int rc =3D 0; >>> - if (parser.rc !=3D SQL_OK) { >>> - /* Tarantool error may be already set with diag. */ >>> - if (parser.rc !=3D SQL_TARANTOOL_ERROR) >>> - diag_set(ClientError, ER_SQL, parser.zErrMsg); >>> - rc =3D -1; >>> - } >>> + if (parser.rc !=3D SQL_OK) >>> + return -1; >> Since now we have only one possible RC, lets remove >> its name and simply check (parser.rc !=3D 0). >> Or, as suggested Konstantin, better replace rc with bool is_aborted = flag. > Do you mind if I do this in a new patch? Sure, but place it before this one. >>> 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 =3D &def->fields[def->field_count - 1]; >>> if (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT && >>> nullable_action !=3D field->nullable_action) { >>> - /* Prevent defining nullable_action many times. */ >>> - const char *err_msg =3D >>> - 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 =3D 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 No, it wasn=E2=80=99t. > cannot be fixed in just parse_context->rc =3D SQL_TARANTOOL_ERROR > and diag_set(). Here it uses tt_printf() in addition to these two > commands. It=E2=80=99s OK. Revert this (and other) change(s), please. Lets use = pure diag_set + abort flag. >>> return; >>> } >>> field->nullable_action =3D 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 =3D space_by_name(tab_name); >>> if (space =3D=3D 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. Don=E2=80=99t waste time now, it was just advice to keep in mind. >>> } >>> if (! rlist_empty(&space->parent_fk_constraint)) { >>> - const char *err_msg =3D >>> - 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. >>=20 >>> @@ -146,11 +145,7 @@ sqlPrepare(sql * db, /* Database handle. */ >>> *ppStmt =3D (sql_stmt *) sParse.pVdbe; >>> } >>>=20 >>> - 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? You don=E2=80=99t have to do it right now, it can wait till 2.2 Just file this issue somewhere. --Apple-Mail=_276A8A4A-A052-4A14-9387-DD1FCAF29400 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On = 26 Feb 2019, at 18:36, Imeev Mergen <imeevma@tarantool.org> wrote:
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 =3D true;

= sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, = expr_list);
- int rc =3D 0;
- if = (parser.rc !=3D SQL_OK) {
- /* Tarantool error may be already = set with diag. */
- if (parser.rc !=3D = SQL_TARANTOOL_ERROR)
- diag_set(ClientError, ER_SQL, = parser.zErrMsg);
- rc =3D -1;
- }
+ = if (parser.rc !=3D SQL_OK)
+ return = -1;
Since now we have only one possible RC, = lets remove
its name and simply check (parser.rc !=3D = 0).
Or, as suggested Konstantin, better replace rc with = bool is_aborted flag.
Do you mind if I do this in a = new patch?

Sure, = but place it before this one.

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 =3D = &def->fields[def->field_count - 1];
if = (field->nullable_action !=3D ON_CONFLICT_ACTION_DEFAULT &&
=     nullable_ac= tion !=3D field->nullable_action) {
- /* = Prevent defining nullable_action many times. */
- const = char *err_msg =3D
- tt_sprintf("NULL declaration for = column '%s' of table "
-    "'%s' has been = already set to '%s'",
-    field->name, = def->name,
-    on_conflict_actio= n_strs[field->
-    nullable_action])= ;
- diag_set(ClientError, ER_SQL, err_msg);
- = = parser->rc =3D SQL_TARANTOOL_ERROR;
- = parser->nErr++;
+ sqlErrorMsg(parser, "NULL = declaration for column '%s' of "\
+     "table = '%s' has been already set to '%s'",
+     field->n= ame, 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

No, = it wasn=E2=80=99t.

cannot be fixed in just parse_context->rc =3D = SQL_TARANTOOL_ERROR
and diag_set(). Here it uses tt_printf() in addition to these = two
commands.

It=E2=80=99s OK. Revert this (and other) = change(s), please. Lets use pure
diag_set + abort = flag.

= return;
}
= field->nullable_action =3D 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 =3D space_by_name(tab_name);
= if (space =3D=3D 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.

Don=E2=80=99t waste time now, it was just advice = to keep in mind.

}
= if (! rlist_empty(&space->parent_fk_constraint)) {
- = = const char *err_msg =3D
- = 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 =3D= (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?

You don=E2=80=99= t have to do it right now, it can wait till 2.2
Just file this = issue somewhere.

= --Apple-Mail=_276A8A4A-A052-4A14-9387-DD1FCAF29400--