[tarantool-patches] Re: [PATCH v2 4/5] sql: remove file zErrMsg of struct Parse

[Imeev Mergen]

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