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

[n.pettik]

> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Thanks for the fixes! See 2 comments below.
> 
>>> 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;
> 
> 1. Are you sure you can save the pointer to the stack value and then
> use it out of the declaration context?
> 
> Here tmp_tab is declared inside {} block, but pointer on it is
> used out the former.

Well,  I'm not so sure about that..

@@ -99,6 +99,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
         */
        bool is_complex = false;
        const char *tab_name = tab_list->a->zName;
+       struct Table tmp_tab;
        if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) {
                space_id = box_space_id_by_name(tab_name,
                                                strlen(tab_name));
@@ -111,7 +112,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                 * wrapper around space_def to support interface.
                 */
                space = space_by_id(space_id);
-               struct Table tmp_tab;

>>> 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,
> 
> 2. clase - clause. Else the patch can not be built.

Oops, I am sorry:

@@ -506,9 +506,9 @@ sqlite3WhereFindTerm(WhereClause * pWC,     /* The WHERE clause to be searched */
  * @retval New struct describing WHERE term.
  */
 static inline struct WhereTerm *
-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)
+where_clause_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)