From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9B1D82722B for ; Mon, 22 Apr 2019 05:53:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id SgRJ6v8wwY8I for ; Mon, 22 Apr 2019 05:53:10 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 05E542722A for ; Mon, 22 Apr 2019 05:53:09 -0400 (EDT) Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1hIVdM-0000UZ-0U for tarantool-patches@freelists.org; Mon, 22 Apr 2019 12:53:08 +0300 Subject: [tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt References: <2b1c4f5e030ffa41069a3839f6ba39826ea739e1.1555072183.git.imeevma@gmail.com> <46cce27c-6a0c-e988-f58c-e5b3456b4775@tarantool.org> From: Imeev Mergen Message-ID: <9c521238-4111-efd2-b32c-9bc980606c0a@tarantool.org> Date: Mon, 22 Apr 2019 12:53:07 +0300 MIME-Version: 1.0 In-Reply-To: <46cce27c-6a0c-e988-f58c-e5b3456b4775@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.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. > >