[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[Imeev Mergen <imeevma@tarantool.org>]

On 4/22/19 11:47 AM, Imeev Mergen wrote:
>
> On 4/15/19 6:19 PM, n.pettik wrote:
>> 'make ... the only errcode of OP_Halt’ ->
>> make … be the only ...
>>
>>> On 12 Apr 2019, at 15:34,imeevma@tarantool.org  wrote:
>>>
>>> After this patch, the only error code that the OP_Halt opcode
>>> will work with is SQL_TARANTOOL_ERROR.
>> So, why do we need it at all now? Let’s use simple flag
>> is_aborted like in parser.
> I don't think we should do is_aborted in the current situation.
> All errors that go through OP_Halt are errors that were detected
> by VDBE means. It can be said that this is a separate type of
> error. Another type of errors go through abort_due_to_error and do
> not work with OP_Halt opcode.
>
> Currently, error handling in OP_Halt is quite complex, so I try to
> make it simpler and clearer. This comes with the diag_set()
> implementation here.
>
> But I think we will add the is_aborted field after reworking errors
> of the other type.
>
Thinking a little, I realized that the idea to add is_aborted is
very good. I will try to do this and send a new patch later.
>>> Part of #4074
>>> ---
>>> src/box/sql/build.c         | 20 --------------------
>>> src/box/sql/expr.c          |  7 ++++---
>>> src/box/sql/fk_constraint.c |  7 +++----
>>> src/box/sql/insert.c        | 29 ++++++++++++++---------------
>>> src/box/sql/sqlInt.h        |  1 -
>>> src/box/sql/vdbe.c          | 23 ++++-------------------
>>> 6 files changed, 25 insertions(+), 62 deletions(-)
>>>
>>>
>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index eb08f7e..ba41837 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * 
>>> pExpr, int target)
>>>                       ON_CONFLICT_ACTION_IGNORE, 0,
>>>                       pExpr->u.zToken, 0);
>>>         } else {
>>> -            sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
>>> -                          pExpr->on_conflict_action,
>>> -                          pExpr->u.zToken, 0, 0);
>>> +            sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
>>> +                      pExpr->on_conflict_action, 0,
>>> +                      pExpr->u.zToken, 0);
>>> +            sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
>> Are there any other options for P5 argument,
>> except for ER_SQL_EXECUTE? If not so, let’s always
>> assume it is ER_SQL_EXECUTE and avoid passing
>> extra argument.
> At the moment OP_Halt works with five errors: ER_TRIGGER_EXISTS,
> ER_NO_SUCH_TRIGGER, ER_CONSTRAINT_EXISTS, ER_NO_SUCH_CONSTRAINT,
> ER_SQL_EXECUTE. In my patch, I added ER_SQL, since some error
> messages had the prefix of this error.
>
> But I can move errcode from p5 to p3. Should I do this?
>
> Also, should I use ER_SQL_EXECUTE instead of ER_SQL? I have to
> edit tests in this case.
>
>