[tarantool-patches] Re: [PATCH v2 09/11] sql: new _trigger space format with space_id
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jun 13 21:53:32 MSK 2018
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.
More information about the Tarantool-patches
mailing list