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 2F351252F7 for ; Mon, 18 Jun 2018 06:44:58 -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 AiCm3Iu8aPh2 for ; Mon, 18 Jun 2018 06:44:58 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 C459224D98 for ; Mon, 18 Jun 2018 06:44:54 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts References: <16d25107-35e6-d6db-d0d5-01a47f1d3909@tarantool.org> <90D2A28D-188B-45EB-89A9-43462A3ECF36@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> Date: Mon, 18 Jun 2018 13:44:52 +0300 MIME-Version: 1.0 In-Reply-To: <90D2A28D-188B-45EB-89A9-43462A3ECF36@tarantool.org> 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: "n.pettik" , tarantool-patches@freelists.org Cc: Kirill Yukhin Hello. Thanks for the patch! See 6 comments below. On 18/06/2018 05:11, n.pettik wrote: > For some reason this patch got to the trunk without review fixes. > Raw version of this patch contains several bugs, so they led to test fails > (sql-tap/analyze6.test.lua and few sql-tap/tkt*). > > I suggest my own fixes. The whole patch is at the end of letter. > > Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete > Issue: https://github.com/tarantool/tarantool/issues/3463 > >> >>> } >>> struct ExprList * >>> @@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id) >>> space_ptr_reg); >>> } >>> +int >>> +sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space) >> >> 2. Few lines above I see emit_open_cursor. Please, try to remove one of them. >> At the worst case you can just inline emit_open_cursor in the single place of >> its usage, and rename sql_emit_open_cursor to emit_open_cursor. > > Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor(). > The only difference is in doing space lookup outside emit_open_cursor(). 1. How about rename this function to parser_emit_open_cursor? According to Tarantool naming convention a method name should be called as 'object_action_subject'. Now we have too many different prefixes of parser methods: sqlite3, sql, parse, parser, . Lets use 'parser'. >> >>> +} >>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c >>> index 2b59130..de4e0c1 100644 >>> --- a/src/box/sql/delete.c >>> +++ b/src/box/sql/delete.c >>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, >>> memset(&nc, 0, sizeof(nc)); >>> nc.pParse = parse; >>> if (tab_list->a[0].pTab == NULL) { >>> - struct Table *t = malloc(sizeof(struct Table)); >>> + struct Table *t = calloc(sizeof(struct Table), 1); >> >> 4. It must the part of the previous patch. > > Refactored this way: > > +++ b/src/box/sql/delete.c > @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > strlen(tab_name)); > if (space_id == BOX_ID_NIL) > goto delete_from_cleanup; > + /* > + * In this case space has been created via Lua > + * facilities, so there is no corresponding entry > + * in table hash. Thus, lets create simple > + * wrapper around space_def to support interface. > + */ > + space = space_by_id(space_id); > + tab_list->a[0].pTab = sql_table_construct_from_space(space); > + if (tab_list->a[0].pTab == NULL) > + goto delete_from_cleanup; 2. As I proposed in the review, why can not we use stack struct Table here? Just declare it at the beginning of the function and here save its pointer. > @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index) > char * > sql_index_affinity_str(struct sqlite3 * db, struct index_def *def) > { > - char *aff; > - /* The first time a column affinity string for a particular index is > - * required, it is allocated and populated here. It is then stored as > - * a member of the Index structure for subsequent use. > - * > - * The column affinity string will eventually be deleted by > - * sqliteDeleteIndex() when the Index structure itself is cleaned > - * up. > - */ > - int nColumn = def->key_def->part_count; > - aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1); > + uint32_t column_count = def->key_def->part_count; > + char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1); > if (aff == NULL) { > sqlite3OomFault(db); > - return 0; > + return NULL; > } 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us. As I see in codeAllEqualityTerms() (where this function is used) affinity is either allocated as sqlite3DbStrDup(pParse->db) or in sql_index_affinity_str() using sqlite3DbMallocRaw(NULL). Different ways of allocating (with db or not) and freeing leads to very surprising assertions sometimes. > - int i; > - struct space *space = space_cache_find(def->space_id); > + struct space *space = space_by_id(def->space_id); > assert(space != NULL); > > - for (i = 0; i < nColumn; i++) { > + for (uint32_t i = 0; i < column_count; i++) { > uint32_t x = def->key_def->parts[i].fieldno; > aff[i] = space->def->fields[x].affinity; > if (aff[i] == AFFINITY_UNDEFINED) > aff[i] = 'A'; > } > - aff[i] = 0; > + aff[column_count] = '\0'; > > return aff; > } > > ======================================================================= > > From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001 > From: Nikita Pettik > Date: Mon, 18 Jun 2018 00:38:55 +0300 > Subject: [PATCH] sql: review fixes for a40287051 > > --- > src/box/sql/analyze.c | 6 +- > src/box/sql/build.c | 27 +---- > src/box/sql/delete.c | 45 +++---- > src/box/sql/expr.c | 9 +- > src/box/sql/fkey.c | 7 +- > src/box/sql/insert.c | 36 +++--- > src/box/sql/resolve.c | 18 +++ > src/box/sql/select.c | 5 +- > src/box/sql/sqliteInt.h | 42 ++++--- > src/box/sql/where.c | 309 +++++++++++++++++++++++++----------------------- > src/box/sql/wherecode.c | 5 +- > 11 files changed, 256 insertions(+), 253 deletions(-) >> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index 23e16189a..e7d9723fa 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -39,6 +39,9 @@ > #include > #include > > +#include "box/box.h" > +#include "box/schema.h" 4. Why do you need these headers? > + > /* > * Walk the expression tree pExpr and increase the aggregate function > * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node. > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index f017384b5..a3a5bec81 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst) > return SQLITE_OK; > } > > -/* > - * Most queries use only a single table (they are not joins) and have > - * simple == constraints against indexed fields. This routine attempts > - * to plan those simple cases using much less ceremony than the > - * general-purpose query planner, and thereby yield faster sqlite3_prepare() > - * times for the common case. > +/** > + * Attempt at finding appropriate terms in WHERE clause. > * > - * Return non-zero on success, if this query can be handled by this > - * no-frills query planner. Return zero if this query needs the > - * general-purpose query planner. > + * @param loop The loop @where belongs to. > + * @param where The WHERE clause to be examined.. > + * @param cursor Cursor number of LHS. > + * @param space_def Def of the space related to WHERE clause. > + * @param index_def Def of the index to be used to satisfy WHERE > + * clause. May be NULL. > + * @retval 0 on success, -1 otherwise. > */ > static int > -sql_where_shortcut(struct WhereLoopBuilder *builder) > +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where, > + int cursor, struct space_def *space_def, > + struct index_def *idx_def) 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms'. > +/** > + * Most queries use only a single table (they are not joins) and > + * have simple == constraints against indexed fields. This > + * routine attempts to plan those simple cases using much less > + * ceremony than the general-purpose query planner, and thereby > + * yield faster sqlite3_prepare() times for the common case. > + * > + * @param builder Where-Loop Builder. > + * @retval Return non-zero on success, i.e. if this query can be > + * handled by this no-frills query planner. Return zero > + * if this query needs the general-purpose query planner. > + */ > +static int > +sql_where_shortcut(struct WhereLoopBuilder *builder) 6. Same. For example, 'where_loop_builder_shortcut'.