[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/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.

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