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 3F5ED25024 for ; Thu, 5 Jul 2018 12:11:38 -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 Y5p1c6qXnKiw for ; Thu, 5 Jul 2018 12:11:38 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 DDBA8216D8 for ; Thu, 5 Jul 2018 12:11:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/2] sql: get rid off sqlite3NestedParse in clean stats References: From: Vladislav Shpilevoy Message-ID: <355d8c55-abc7-a088-8dba-6fb3bff32b51@tarantool.org> Date: Thu, 5 Jul 2018 19:11:35 +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: tarantool-patches@freelists.org, Kirill Shcherbatov Hello. Thanks for the patch! I have pushed my review fixes on the branch. Please, squash, if you agree, and debug if the tests fail. Below you may find 3 comments that I fixed. Style violations I did not mentioned here. Just fixed on the branch. Also I have found that vdbe_emit_open_cursor() has the second parameter named 'index_id', but in some places the function takes real index_id, in other places it takes tnum, and in vdbe it is interpreted as tnum. Please, fix this mess in a separate commit. I think, we should always pass index_id. On 04/07/2018 20:17, Kirill Shcherbatov wrote: > Now we manually generate AST structures to drop outdated > stats from _sql_stat1 and _sql_stat4 spaces instead of > starting sqlite3NestedParse. This function become totally > useless and could be removed. > > Part of #3496. > --- > src/box/sql/analyze.c | 39 +++++++++++------------ > src/box/sql/build.c | 82 ++++++++++++++++++++++++++++++++++++------------- > src/box/sql/sqliteInt.h | 13 ++++++++ > 3 files changed, 94 insertions(+), 40 deletions(-) > > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 5f73f02..e08c151 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -116,7 +116,7 @@ > #include "tarantoolInt.h" > #include "vdbeInt.h" > > -/* > +/** > * This routine generates code that opens the sql_statN tables. > * The _sql_stat1 table is always relevant. _sql_stat4 is only opened when > * appropriate compile-time options are provided. > @@ -160,10 +163,9 @@ openStatTable(Parse * pParse, /* Parsing context */ > assert(pStat != NULL); > aRoot[i] = pStat->tnum; > aCreateTbl[i] = 0; 1. Unused array. 2. You do not need to lookup pStat for tnum in non-debug build since you know stat tables id from schema_def.h. This HashFind is useful for debug assertion only. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0072f84..ac53906 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2050,6 +2050,63 @@ sql_store_select(struct Parse *parse_context, struct Select *select) > } > > /** > + * Create expression record of with struct ID EQ STRING. > + * > + * @param parse The parsing context. > + * @param col_type_name Name of column. > + * @param col_name Name of row. > + * @retval not NULL on success. > + * @retval NULL on failure. > + */ > +static struct Expr * > +sql_id_eq_str_expr(struct Parse *parse, const char *col_type_name, > + const char *col_name) > +{ > + struct sqlite3 *db = parse->db; > + > + struct Expr *col_type_expr = > + sqlite3Expr(db, TK_ID, col_type_name); > + struct Expr *col_name_expr = > + sqlite3Expr(db, TK_STRING, col_name); > + struct Expr *col_eq_expr = > + sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr); > + if (col_type_expr == NULL || col_name_expr == NULL) { 3. If col_eq_expr == NULL here, then col_type/name_expr leak. > + sql_expr_delete(db, col_eq_expr, false); > + col_eq_expr = NULL; > + } > + return col_eq_expr; > +} > +