[tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name

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

On 19/11/2018 13:27, Vladislav Shpilevoy wrote:
> Hi! Thanks for the rebase! See 4 comments below.
> 
>> commit b9792dfc183d7d0851f3492c8ba9e2f7c46fe385
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date:   Fri Aug 31 12:37:48 2018 +0300
>>
>>      sql: hold in stat tables space/index id instead of name
>>
>>      To avoid problems with table and index renaming it is good idea
>>      to save ids of tables and indexes instead of their names. Ids of
>>      tables and indexes are fixed values.
>>
>>      Closes #3242
>>      Closes #2962
>>
>> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
>> index b7789d6..538e635 100644
>> Binary files a/src/box/bootstrap.snap and b/src/box/bootstrap.snap differ
>> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
>> index 3d9acc9..3ce586b 100644
>> --- a/src/box/lua/upgrade.lua
>> +++ b/src/box/lua/upgrade.lua
>> @@ -550,11 +550,11 @@ local function upgrade_to_2_1_0()
>>       _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
>>                     {{1, 'unsigned'}}}
>>
>> -    local stat1_ft = {{name='tbl', type='string'},
>> -                      {name='idx', type='string'},
>> +    local stat1_ft = {{name='space_id', type='unsigned'},
>> +                      {name='index_id', type='unsigned'},
>>                         {name='stat', type='string'}}
>> -    local stat4_ft = {{name='tbl', type='string'},
>> -                      {name='idx', type='string'},
>> +    local stat4_ft = {{name='space_id', type='unsigned'},
>> +                      {name='index_id', type='unsigned'},
>>                         {name='neq', type='string'},
>>                         {name='nlt', type='string'},
>>                         {name='ndlt', type='string'}
> 
> 1. Unfortunately, as I said earlier, you can not change the
> past. 2.1.0 is already released, so you should create upgrade_to_2_2_0()
> function and here update stat spaces format.

Sorry, 2.1.1, not 2.2.0.

> 
>> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
>> index e786c90..aa5d349 100644
>> --- a/src/box/sql/analyze.c
>> +++ b/src/box/sql/analyze.c
>> @@ -800,13 +800,13 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
>>       assert(space->index_count != 0);
>>       struct Vdbe *v = sqlite3GetVdbe(parse);
>>       assert(v != NULL);
>> -    const char *tab_name = space_name(space);
>> +    MAYBE_UNUSED const char *tab_name = space_name(space);
> 
> 2. Why unused?
> 
>>       sqlite3VdbeAddOp4(v, OP_IteratorOpen, tab_cursor, 0, 0, (void *) space,
>>                 P4_SPACEPTR);
>> -    sqlite3VdbeLoadString(v, tab_name_reg, space->def->name);
>> +    sqlite3VdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg);
>>       for (uint32_t j = 0; j < space->index_count; ++j) {
>>           struct index *idx = space->index[j];
>> -        const char *idx_name;
>> +        MAYBE_UNUSED const char *idx_name;
> 
> 3. Why?
> 
>>           /*
>>            * Primary indexes feature automatically generated
>>            * names. Thus, for the sake of clarity, use
>> @@ -1225,23 +1224,14 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
>>       struct analysis_index_info *info = (struct analysis_index_info *) data;
>>       assert(info->stats != NULL);
>>       struct index_stat *stat = &info->stats[info->index_count++];
>> -    struct space *space = space_by_name(argv[0]);
>> -    if (space == NULL)
>> +    uint32_t space_id = atoll(argv[0]);
>> +    if (space_id == 0)
>>           return -1;
>> +    struct space *space = space_by_id(space_id);
>> +    assert(space != NULL);
>>       struct index *index;
>> -    uint32_t iid = box_index_id_by_name(space->def->id, argv[1],
>> -                        strlen(argv[1]));
>> -    /*
>> -     * Convention is if index's name matches with space's
>> -     * one, then it is primary index.
>> -     */
>> -    if (iid != BOX_ID_NIL) {
>> -        index = space_index(space, iid);
>> -    } else {
>> -        if (sqlite3_stricmp(argv[0], argv[1]) != 0)
>> -            return -1;
>> -        index = space_index(space, 0);
>> -    }
>> +    uint32_t iid = atoll(argv[1]);
>> +    index = space_index(space, iid);
> 
> 4.1. Why in some places d oyou check result of
> atoll and sqlite3_column_int, but in others do not?
> 
>>       assert(index != NULL);
>>       /*
>>        * Additional field is used to describe total
>> @@ -1387,24 +1381,14 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
>>           goto finalize;
>>       uint32_t current_idx_count = 0;
>>       while (sqlite3_step(stmt) == SQLITE_ROW) {
>> -        const char *space_name = (char *)sqlite3_column_text(stmt, 0);
>> -        if (space_name == NULL)
>> -            continue;
>> -        const char *index_name = (char *)sqlite3_column_text(stmt, 1);
>> -        if (index_name == NULL)
>> -            continue;
>> -        uint32_t sample_count = sqlite3_column_int(stmt, 2);
>> -        struct space *space = space_by_name(space_name);
>> +        uint32_t space_id = sqlite3_column_int(stmt, 0);
>> +        assert(space_id != 0);
>> +        struct space *space = space_by_id(space_id);
> 
> 4.2. Here you do not check, but ...
> 
>>           assert(space != NULL);
>> -        struct index *index;
>> @@ -1537,24 +1509,13 @@ load_stat_to_index(struct sqlite3 *db, const char *sql_select_load,
>>           return -1;
>>       uint32_t current_idx_count = 0;
>>       while (sqlite3_step(stmt) == SQLITE_ROW) {
>> -        const char *space_name = (char *)sqlite3_column_text(stmt, 0);
>> -        if (space_name == NULL)
>> -            continue;
>> -        const char *index_name = (char *)sqlite3_column_text(stmt, 1);
>> -        if (index_name == NULL)
>> -            continue;
>> -        struct space *space = space_by_name(space_name);
>> +        uint32_t space_id = sqlite3_column_int(stmt, 0);
>> +        if (space_id == 0)
>> +            return -1;
> 
> 4.3. ... here you check again.
> 
>> +        struct space *space = space_by_id(space_id);
>>           assert(space != NULL);
>> -        struct index *index;
>> -        uint32_t iid = box_index_id_by_name(space->def->id, index_name,
>> -                            strlen(index_name));
>> -        if (iid != BOX_ID_NIL) {
>> -            index = space_index(space, iid);
>> -        } else {
>> -            if (sqlite3_stricmp(space_name, index_name) != 0)
>> -                return -1;
>> -            index = space_index(space, 0);
>> -        }
>> +        uint32_t iid = sqlite3_column_int(stmt, 1);
> 
> 4.4. And here you don't ...
> 
>> +        struct index *index = space_index(space, iid);
>>           assert(index != NULL);
>>           free(index->def->opts.stat);
>>           index->def->opts.stat = stats[current_idx_count++];
>