[tarantool-patches] Re: [PATCH v2 09/11] sql: new _trigger space format with space_id

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

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.