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 = 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?

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?