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

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

Now LGTM.

On 18/06/2018 17:00, n.pettik wrote:
> 
>> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy@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)
> 
>