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 191F82EEBD for ; Sun, 2 Jun 2019 12:35:09 -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 o7jXsbGGX-m6 for ; Sun, 2 Jun 2019 12:35:09 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 C76992EEB7 for ; Sun, 2 Jun 2019 12:35:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/9] sql: rework diag_set() in OP_Halt References: <116d34dc76fbe262015c0079bba2dc00f6a40c8c.1559043251.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <0208a503-1a04-fb78-cf8b-563185c5ab42@tarantool.org> Date: Sun, 2 Jun 2019 18:35:03 +0200 MIME-Version: 1.0 In-Reply-To: <116d34dc76fbe262015c0079bba2dc00f6a40c8c.1559043251.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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, imeevma@tarantool.org 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. > 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);