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 8D2AC25EA3 for ; Mon, 22 Apr 2019 04:47:15 -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 kfzCMR7-BgNb for ; Mon, 22 Apr 2019 04:47:15 -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 153F825AE1 for ; Mon, 22 Apr 2019 04:47:14 -0400 (EDT) From: Imeev Mergen 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> Message-ID: <46cce27c-6a0c-e988-f58c-e5b3456b4775@tarantool.org> Date: Mon, 22 Apr 2019 11:47:11 +0300 MIME-Version: 1.0 In-Reply-To: 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: "n.pettik" , tarantool-patches@freelists.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.