[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jun 18 17:17:31 MSK 2018
Now LGTM.
On 18/06/2018 17:00, n.pettik wrote:
>
>> 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)
>
>
More information about the Tarantool-patches
mailing list