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 CE23C2F549 for ; Mon, 3 Jun 2019 04:41:52 -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 LERRep52BvXB for ; Mon, 3 Jun 2019 04:41:52 -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 DEADC2FA7F for ; Mon, 3 Jun 2019 04:41:44 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/9] sql: rework diag_set() in OP_Halt References: <116d34dc76fbe262015c0079bba2dc00f6a40c8c.1559043251.git.imeevma@gmail.com> <0208a503-1a04-fb78-cf8b-563185c5ab42@tarantool.org> From: Imeev Mergen Message-ID: <2f731c49-b8c4-f25b-a0c8-b5dcf382b60c@tarantool.org> Date: Mon, 3 Jun 2019 11:41:42 +0300 MIME-Version: 1.0 In-Reply-To: <0208a503-1a04-fb78-cf8b-563185c5ab42@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: Vladislav Shpilevoy , tarantool-patches@freelists.org Hi! Thank you for the review! My answers below. Also, I think I'm going to drop this patch, since I plan to move the error code from p5 to p2 in the next patch. On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote: > > On 28/05/2019 14:39, imeevma@tarantool.org wrote: >> Prior to this patch, the way to set Tarantool error in OP_Halt was >> too universal. It was possible to set a description of the error >> that does not match its errcode. > There was a concrete reason, why it was possible - because different > error codes have different arguments of various types, and the only way > to set an error at parsing stage is to allow to set arbitrary error > message to any error code. Without '...', va_arg etc. Besides, we could > use it to set correct line number and function name in future. Now you > use diag_set, which restricts us. > > So why do you need that patch? We will need to revert it when an error > appears requiring more than one argument, or an argument of not > const char * type. That will definitely happen. For some reason, manually creating an error message looks a bit wrong to me. How about creating a new macro to create such messages, for example, diag_prepare(), which takes the same arguments as diag_set() and returns the complete error message? >> This change will also make it >> easier to work with an error in OP_Halt, since you no longer need >> to create a complete error message.> >> Example of wrong error code: >> ... >> >> tarantool> box.execute('select 1 limit true') >> --- >> - error: Only positive integers are allowed in the LIMIT clause >> ... >> >> tarantool> box.error.last().code >> --- >> - 0 >> ... >> --- > This is not because of box_error_set usage. > > tarantool> box.error({code = 123, reason = 'test'}) > --- > - error: test > ... > > tarantool> box.error.last().code > --- > - 123 > ... > > This example uses box_error_set() too, but the error > code is kept. The bug in SQL could be fixed in a few lines, > and you did it. But somewhy decided to break generic > error creation alongside. > > The whole patch could be shrunk to this (+ tests): > >> diff --git a/src/box/sql/select.c b/src/box/sql/select.c >> index d3472a9..3f0b540 100644 >> --- a/src/box/sql/select.c >> +++ b/src/box/sql/select.c >> @@ -2116,6 +2116,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) >> 0, 0, >> wrong_limit_error, >> P4_STATIC); >> + sqlVdbeChangeP5(v, ER_SQL_EXECUTE);> >> sqlVdbeResolveLabel(v, positive_limit_label); >> VdbeCoverage(v); >> @@ -2178,6 +2178,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) >> 0, 0, >> wrong_offset_error, >> P4_STATIC); >> + sqlVdbeChangeP5(v, ER_SQL_EXECUTE); >> >> sqlVdbeResolveLabel(v, positive_offset_label); >> sqlReleaseTempReg(pParse, r1); True, but I think I won't need this after I move errcode from p5 to p2 in the next patch.