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 9F99025B1A for ; Wed, 13 Jun 2018 14:53:35 -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 kHHdIJA38GsK for ; Wed, 13 Jun 2018 14:53:35 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 5C20E259A5 for ; Wed, 13 Jun 2018 14:53:35 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 09/11] sql: new _trigger space format with space_id References: From: Vladislav Shpilevoy Message-ID: <36b21e2d-d459-d373-7c56-02537de5fdd9@tarantool.org> Date: Wed, 13 Jun 2018 21:53:32 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Kirill Shcherbatov Thanks for the patch! Almost ok. See 5 minor comments below. On 09/06/2018 12:32, Kirill Shcherbatov wrote: > As we would like to lookup triggers by space_id in future > on space deletion to delete assocoated _trigger tuples we 1. "assocoated" - typo. > need to introduce new field space_id as secondary key. > > Part of #3273. > --- > src/box/bootstrap.snap | Bin 1698 -> 1704 bytes > src/box/lua/upgrade.lua | 6 +++++- > src/box/sql.c | 18 +++++++++++------- > src/box/sql/sqliteInt.h | 2 ++ > src/box/sql/trigger.c | 8 +++++--- > test/sql/gh2141-delete-trigger-drop-table.result | 4 ++-- > test/sql/gh2141-delete-trigger-drop-table.test.lua | 4 ++-- > test/sql/persistency.result | 8 ++++---- > test/sql/persistency.test.lua | 8 ++++---- > 9 files changed, 35 insertions(+), 23 deletions(-) > > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index 18bfaa9..e54b394 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua > @@ -477,12 +477,16 @@ local function upgrade_to_2_1_0() > > log.info("create space _trigger") > local format = {{name='name', type='string'}, > + {name='space_id', type='unsigned'}, > {name='opts', type='map'}} > _space:insert{_trigger.id, ADMIN, '_trigger', 'memtx', 0, MAP, format} > > log.info("create index primary on _trigger") > _index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true }, > - {{0, 'string'}}} > + {{0, 'string'}} } 2. Unnecessary diff. Either do this: {data, data}, or this { data, data }. Not this: {data, data }. Same below. > + log.info("create index secondary on _trigger") > + _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false }, > + {{1, 'unsigned'}} } > > local stat1_ft = {{name='tbl', type='string'}, > {name='idx', type='string'}, > diff --git a/src/box/sql.c b/src/box/sql.c > index 599cb60..0ba0346 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -693,16 +696,17 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name, > uint32_t trigger_stmt_new_len = trigger_stmt_len + new_table_name_len - > old_table_name_len + 2 * (!is_quoted); > assert(trigger_stmt_new_len > 0); > - key_len = mp_sizeof_array(2) + mp_sizeof_str(trig_name_len) + > + key_len = mp_sizeof_array(3) + mp_sizeof_str(trig_name_len) + > mp_sizeof_map(1) + mp_sizeof_str(3) + > - mp_sizeof_str(trigger_stmt_new_len); > + mp_sizeof_str(trigger_stmt_new_len) + mp_sizeof_uint(space_id); 3. Out of 80. > char *new_tuple = (char*)region_alloc(&fiber()->gc, key_len); > if (new_tuple == NULL) { > diag_set(OutOfMemory, key_len, "region_alloc", "new_tuple"); > return SQL_TARANTOOL_ERROR; > } > - char *new_tuple_end = mp_encode_array(new_tuple, 2); > + char *new_tuple_end = mp_encode_array(new_tuple, 3); > new_tuple_end = mp_encode_str(new_tuple_end, trig_name, trig_name_len); > + new_tuple_end = mp_encode_uint(new_tuple_end, space_id); > new_tuple_end = mp_encode_map(new_tuple_end, 1); > new_tuple_end = mp_encode_str(new_tuple_end, "sql", 3); > new_tuple_end = mp_encode_str(new_tuple_end, trigger_stmt, > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index ecbd573..6ca59c4 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -3032,6 +3032,8 @@ struct Parse { > */ > struct Trigger { > char *zName; /* The name of the trigger */ > + /** The ID of space the trigger is refer to. */ 4. 'is refer to' is incorrect construction. Maybe did you mean 'refers to'? > + uint32_t space_id; > char *table; /* The table or view to which the trigger applies */ > u8 op; /* One of TK_DELETE, TK_UPDATE, TK_INSERT */ > u8 tr_tm; /* One of TRIGGER_BEFORE, TRIGGER_AFTER */ 5. How about to test space_id is stored in _trigger? I mean test box.space._trigger:select{} full result. Not two columns only.