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 2AB322584E for ; Mon, 18 Jun 2018 06:51:31 -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 hXA5VGfTNxnF for ; Mon, 18 Jun 2018 06:51:31 -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 B6A56257EA for ; Mon, 18 Jun 2018 06:51:30 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts From: Vladislav Shpilevoy References: <16d25107-35e6-d6db-d0d5-01a47f1d3909@tarantool.org> <90D2A28D-188B-45EB-89A9-43462A3ECF36@tarantool.org> <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> Message-ID: Date: Mon, 18 Jun 2018 13:51:25 +0300 MIME-Version: 1.0 In-Reply-To: <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 Forgot to say: I have pushed some other minor fixes on the branch. On 18/06/2018 13:44, Vladislav Shpilevoy wrote: > 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'. > >