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

["n.pettik" <korablev@tarantool.org>]

Thanks for fixes, I have squashed them.

> 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’.

Ok, but I decided to call it vdbe_emit_open_cursor() since all staff connected
with code generation rather belongs to vdbe:

+++ b/src/box/sql/analyze.c
@@ -178,7 +178,7 @@ openStatTable(Parse * pParse,       /* Parsing context */
        for (i = 0; aTable[i]; i++) {
                struct space *space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
-               sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
+               vdbe_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);

+++ b/src/box/sql/build.c
@@ -1181,8 +1181,8 @@ space_checks_expr_list(uint32_t space_id)
 }
 
 int
-sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
-                    struct space *space)
+vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+                     struct space *space)
 {
        assert(space != NULL);
        struct Vdbe *vdbe = parse_context->pVdbe;
@@ -2651,7 +2651,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
                sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
                                  0);
        struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
-       sql_emit_open_cursor(pParse, iIdx, tnum, space);
+       vdbe_emit_open_cursor(pParse, iIdx, tnum, space);


+++ b/src/box/sql/expr.c
@@ -2489,8 +2489,8 @@ sqlite3FindInIndex(Parse * pParse,        /* Parsing context */
                                                          P4_DYNAMIC);
                                        struct space *space =
                                                space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
-                                       sql_emit_open_cursor(pParse, iTab,
-                                                            pIdx->tnum, space);
+                                       vdbe_emit_open_cursor(pParse, iTab,
+                                                             pIdx->tnum, space);

+++ b/src/box/sql/insert.c
@@ -55,7 +55,7 @@ sqlite3OpenTable(Parse * pParse,      /* Generate code into this VDBE */
        assert(pPk != 0);
        assert(pPk->tnum == pTab->tnum);
        struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
-       sql_emit_open_cursor(pParse, iCur, pPk->tnum, space);
+       vdbe_emit_open_cursor(pParse, iCur, pPk->tnum, space);

@@ -1943,11 +1941,11 @@ xferOptimization(Parse * pParse,        /* Parser context */
                assert(pSrcIdx);
                struct space *src_space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
-               sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+               vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
                VdbeComment((v, "%s", pSrcIdx->zName));
                struct space *dest_space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
-               sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+               vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);

+++ b/src/box/sql/select.c
@@ -6107,9 +6107,9 @@ sqlite3Select(Parse * pParse,             /* The parser context */
                                 * Open the cursor, execute the OP_Count,
                                 * close the cursor.
                                 */
-                               sql_emit_open_cursor(pParse, cursor,
-                                                    space->def->id << 10,
-                                                    space);
+                               vdbe_emit_open_cursor(pParse, cursor,
+                                                     space->def->id << 10,
+                                                     space);

+++ b/src/box/sql/sqliteInt.h
@@ -3564,8 +3564,8 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
  * @retval address of last opcode.
  */
 int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
-                    struct space *space);
+vdbe_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
+                     struct space *space);


>>> 
>>>> +}
>>>> 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.

Well, OK it is possible:

+++ b/src/box/sql/delete.c
@@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                 * 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;
+               struct Table tmp_tab;
+               memset(&tmp_tab, 0, sizeof(tmp_tab));
+               tmp_tab.def = space->def;
+               /* Prevent from freeing memory in DeleteTable. */
+               tmp_tab.nTabRef = 2;
+               tab_list->a[0].pTab = &tmp_tab;

-struct Table *
-sql_table_construct_from_space(const struct space *space)
-{
-       assert(space != NULL);
-       struct Table *table = calloc(1, sizeof(*table));
-       if (table == NULL) {
-               diag_set(OutOfMemory, sizeof(table), "calloc", "table");
-               return NULL;
-       }
-       table->def = space_def_dup(space->def);
-       if (table->def == NULL)
-               return NULL;
-       return table;
-}
-

-
-/**
- * Create wrapper around space_def of the given space.
- * This routine is required to support original SQLite interface,
- * which accepts struct Table and struct Index DD arguments.
- * Both table and space def are allocated on heap.
- *
- * @param space Space to be prototype.
- * @retval New instance of struct Table allocated on malloc,
- *         or NULL in case of OOM.
- */
-struct Table *
-sql_table_construct_from_space(const struct space *space);
-

> 
> 
>> @@ -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.

Ok:

@@ -108,11 +108,9 @@ char *
 sql_index_affinity_str(struct sqlite3 *db, struct index_def *def)
 {
        uint32_t column_count = def->key_def->part_count;
-       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
-       if (aff == NULL) {
-               sqlite3OomFault(db);
+       char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1);
+       if (aff == NULL)
                return NULL;
-       }

>>  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?

Looks like I forgot to remove them. Anyway, in your fixes you removed them. Thx.

>>   */
>>  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’.

-sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
-                   Bitmask is_ready, u32 op, struct space_def *space_def,
-                   struct key_def *key_def)
+where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
+               Bitmask is_ready, u32 op, struct space_def *space_def,
+               struct key_def *key_def)

@@ -4133,7 +4133,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
  * @retval 0 on success, -1 otherwise.
  */
 static int
-where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
+where_loop_assign_terms(struct WhereLoop *loop, struct WhereClause *where,
                        int cursor, struct space_def *space_def,
                        struct index_def *idx_def)

@@ -4144,9 +4144,9 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
        uint32_t i;
        for (i = 0; i < column_count; ++i) {
                struct WhereTerm *term =
-                       sql_where_find_term(where, cursor, i, 0, WO_EQ,
-                                           space_def, idx_def != NULL ?
-                                           idx_def->key_def : NULL);
+                       where_clause_find_term(where, cursor, i, 0, WO_EQ,
+                                              space_def, idx_def != NULL ?
+                                              idx_def->key_def : NULL);

@@ -4178,7 +4178,7 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
  *         if this query needs the general-purpose query planner.
  */
 static int
-sql_where_shortcut(struct WhereLoopBuilder *builder)
+where_loop_builder_shortcut(struct WhereLoopBuilder *builder)
 {
        struct WhereInfo *where_info = builder->pWInfo;
        if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE)
@@ -4214,7 +4214,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
                                        space->index[i]->def;
                                if (!idx_def->opts.is_unique)
                                        continue;
-                               if (where_assign_loop_terms(loop, clause,
+                               if (where_loop_assign_terms(loop, clause,
                                                            cursor, space_def,
                                                            idx_def) == 0)
                                        break;
@@ -4222,7 +4222,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
                } else {
                        /* Space is ephemeral. */
                        assert(space_def->id == 0);
-                       where_assign_loop_terms(loop, clause, cursor,
+                       where_loop_assign_terms(loop, clause, cursor,
                                                space_def, NULL);

@@ -4537,7 +4537,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
        }
 #endif
 
-       if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
+       if (nTabList != 1 || where_loop_builder_shortcut(&sWLB) == 0) {

> 
>> +/**
>> + * 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’.

Fixed. See code above.