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 C7C0F27DCF for ; Fri, 22 Feb 2019 07:43:44 -0500 (EST) 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 Kbt8t2GXVh8e for ; Fri, 22 Feb 2019 07:43:44 -0500 (EST) 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 7A81B2661C for ; Fri, 22 Feb 2019 07:43:44 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 0/4] sql: store regular identifiers in case-normal form References: From: Vladislav Shpilevoy Message-ID: <546dab2d-9905-128c-8228-b0d7bc4168f7@tarantool.org> Date: Fri, 22 Feb 2019 15:43:42 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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: Kirill Shcherbatov , tarantool-patches@freelists.org On 22/02/2019 15:38, Kirill Shcherbatov wrote: >> Talking about the patchset - I do not understand a point of >> the first 3 patches. I see, that the only reason why you >> pass the parser is a wish to set nErr. Why instead not to just >> do diag_set, return -1, and check for that in all upper functions, >> until parser is already in one of them? This how all box functions >> work with SQL right now - they do not take Vdbe, nor Parse. They >> just set diag and return -1. > > The problem is that when the family of functions calling sql_normalize_name return NULL, > it is assumed that a memory allocation error has occurred, although this is not the case. Then stop this assumption. Set diag_set(OutOfMemory) inside this function. > The API for working with expressions is heterogeneous, and in some cases already accepts Parser - > for example sqlPExpr, sqlPExprAddSelect.> Moreover, in the same way, to set a third-party error it is used in sql Expr: see sqlExprCheckHeight > that set> sqlErrorMsg(pParse, "Expression tree is too large (maximum depth %d)", mxHeight); > Thus, I do not introduce new practices to the API, but on the contrary, bring it to a more consistent state. Again, you rolled back our progress of getting rid of nErr. You should not insert it deeper into the API. Please, do not. > > The alternative that you propose to use the forced setting SQL_TARANTOOL_ERROR (in some cases) three functions. sql_normalize_name always should mean tarantool error. Even on failed sqlite3StrDup you can set diag. > the stack if the function returned NULL and !db.mallocFailed. This will work, however, for the same sqlPExpr analogy > will be incorrect: it is not true that if the expression construction function returned NULL, a Tarantool error occurred. > > This will confuse the already conflicting SQL code. >