[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jun 18 13:51:25 MSK 2018
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, <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 at 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'.
>
>
More information about the Tarantool-patches
mailing list