From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7728B25369 for ; Mon, 18 Jun 2018 10:17:33 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id C8pa7sk8293x for ; Mon, 18 Jun 2018 10:17:33 -0400 (EDT) Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 376BE25251 for ; Mon, 18 Jun 2018 10:17:33 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts References: <16d25107-35e6-d6db-d0d5-01a47f1d3909@tarantool.org> <90D2A28D-188B-45EB-89A9-43462A3ECF36@tarantool.org> <5b82e05e-3977-5189-7291-c302712dda58@tarantool.org> <8E3011EE-AF57-4055-AE08-58DABDB1CC40@tarantool.org> From: Vladislav Shpilevoy Message-ID: <70100bdf-7475-d3f3-ef48-0a6f6eea4c9d@tarantool.org> Date: Mon, 18 Jun 2018 17:17:31 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, "n.pettik" Now LGTM. On 18/06/2018 17:00, n.pettik wrote: > >> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy 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) > >