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 388E22845A for ; Fri, 22 Feb 2019 07:38:59 -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 uWP4ieVIb6nv for ; Fri, 22 Feb 2019 07:38:59 -0500 (EST) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 DBB2428458 for ; Fri, 22 Feb 2019 07:38:58 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 0/4] sql: store regular identifiers in case-normal form References: From: Kirill Shcherbatov Message-ID: Date: Fri, 22 Feb 2019 15:38:56 +0300 MIME-Version: 1.0 In-Reply-To: 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, Vladislav Shpilevoy > 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. 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. The alternative that you propose to use the forced setting SQL_TARANTOOL_ERROR (in some cases) three functions. 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.