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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

On 13.06.2018 21:53, Vladislav Shpilevoy wrote:
> 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.
fixed

> 2. Unnecessary diff. Either do this: {data, data}, or this { data, data }.
> Not this: {data, data }. Same below.
     _index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true },
-                  {{0, 'string'}} }
+                  {{0, 'string'}}}
     log.info("create index secondary on _trigger")
     _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
-                  {{1, 'unsigned'}} }
+                  {{1, 'unsigned'}}}


> 3. Out of 80.
-                 mp_sizeof_str(trigger_stmt_new_len) + mp_sizeof_uint(space_id);
+                 mp_sizeof_str(trigger_stmt_new_len) +
+                 mp_sizeof_uint(space_id);

> 4. 'is refer to' is incorrect construction. Maybe did you mean 'refers to'?
-       /** The ID of space the trigger is refer to. */
+       /** The ID of space the trigger refers to. */

> 5. How about to test space_id is stored in _trigger? I mean test
> box.space._trigger:select{} full result. Not two columns only.
--- a/test/sql/upgrade.test.lua
+++ b/test/sql/upgrade.test.lua
@@ -25,9 +25,16 @@ box.space._index:get({box.space._space.index['name']:get('T1').id, 0})
 box.sql.execute("CREATE TABLE t(x INTEGER PRIMARY KEY);")
 box.sql.execute("CREATE TABLE t_out(x INTEGER PRIMARY KEY);")
 box.sql.execute("CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1); END;")
+box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2); END;")
 box.space._space.index['name']:get('T')
 box.space._space.index['name']:get('T_OUT')
-box.space._trigger:get('T1T')
+t1t = box.space._trigger:get('T1T')
+t2t = box.space._trigger:get('T2T')
+t1t.name
+t1t.opts
+t2t.name
+t2t.opts
+assert(t1t.id == t2t.id)


And some extra changed required by next message review:

+++ b/src/box/schema_def.h
@@ -217,6 +217,12 @@ enum {
        BOX_SPACE_SEQUENCE_FIELD_IS_GENERATED = 2,
 };
 
+enum {
+       BOX_TRIGGER_FIELD_NAME = 0,
+       BOX_TRIGGER_FIELD_SPACE_ID = 1,
+       BOX_TRIGGER_FIELD_OPTS = 2,
+};
+

@@ -1320,7 +1320,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
                ptr = mp_decode_str(&field, &len);
                name = strndup(ptr, len);
 
-               field = tuple_field(tuple, 2);
+               field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);


@@ -670,10 +670,10 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
                return SQL_TARANTOOL_ERROR;
        assert(tuple != NULL);
        assert(tuple_field_count(tuple) == 3);
-       const char *field = box_tuple_field(tuple, 1);
+       const char *field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_SPACE_ID);
        assert(mp_typeof(*field) == MP_UINT);
        uint32_t space_id = mp_decode_uint(&field);
-       field = box_tuple_field(tuple, 2);
+       field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);