[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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, <no_prefix>. 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 <korablev@tarantool.org>
> 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 <stdlib.h>
>   #include <string.h>
>   
> +#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'.