[tarantool-patches] Re: [PATCH v2 4/5] sql: remove file zErrMsg of struct Parse
n.pettik
korablev at tarantool.org
Tue Feb 26 21:17:37 MSK 2019
> On 26 Feb 2019, at 18:36, Imeev Mergen <imeevma at 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?
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 = &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
No, it wasn’t.
> 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.
It’s OK. Revert this (and other) change(s), please. Lets use pure
diag_set + abort flag.
>>> 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.
Don’t waste time now, it was just advice to keep in mind.
>>> }
>>> 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?
You don’t have to do it right now, it can wait till 2.2
Just file this issue somewhere.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20190226/dc00d2c4/attachment.html>
More information about the Tarantool-patches
mailing list