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 E94622877A for ; Mon, 19 Nov 2018 05:38:19 -0500 (EST) 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 kK0BZ9vCx4vU for ; Mon, 19 Nov 2018 05:38:19 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 7764828571 for ; Mon, 19 Nov 2018 05:38:19 -0500 (EST) Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1gOgwb-0001IM-Qh for tarantool-patches@freelists.org; Mon, 19 Nov 2018 13:38:18 +0300 Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name References: <1f1960f842e9443511b4bf2712a0b79bd7fb0764.1535711802.git.imeevma@gmail.com> <0bbf0c18-9d9b-4849-5a01-24729ab08468@tarantool.org> <70A5C3C7-8069-4FF0-A653-46B3B82C9716@tarantool.org> <1538498570.8155792@f528.i.mail.ru> <16CEE3B0-B6FC-4BE0-B381-704AF1794585@tarantool.org> <73C65C49-8FB1-4331-B096-6865F9145730@tarantool.org> <207b4057-eaab-b3bd-1bec-bff9111c0743@tarantool.org> <2b509f61-9145-c1fa-4e1d-a734f615479e@tarantool.org> From: Vladislav Shpilevoy Message-ID: <170881c7-7d26-8a87-9cd8-433a8ccd1055@tarantool.org> Date: Mon, 19 Nov 2018 13:38:17 +0300 MIME-Version: 1.0 In-Reply-To: <2b509f61-9145-c1fa-4e1d-a734f615479e@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 On 19/11/2018 13:27, Vladislav Shpilevoy wrote: > Hi! Thanks for the rebase! See 4 comments below. > >> commit b9792dfc183d7d0851f3492c8ba9e2f7c46fe385 >> Author: Mergen Imeev >> 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++]; >